Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn when second argument is passed to useContext #14729

Merged
merged 1 commit into from
Jan 31, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 30, 2019

This leaves us the ability to later change its meaning. It also warns early avoids confusing cases like [].map(useContext) which accidentally supplies observedBits (#14708 (comment)).

Internal readContext on the dispatcher doesn't have the warning.

@@ -613,8 +613,114 @@ describe('ReactDOMServerHooks', () => {
});

describe('useContext', () => {
itThrowsWhenRendering(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The diff confused git. This is not a new test.

All I did is move the useContext() test with bits into readContext() section below.

},
);

itRenders('warns when bitmask is passed to useContext', async render => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new test.

itRenders(
'can use the same context multiple times in the same function',
'can read the same context multiple times in the same function',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is existing test which I made use readContext instead.

@@ -1300,81 +1324,7 @@ describe('ReactNewContext', () => {
});

describe('readContext', () => {
// Context consumer bails out on propagating "deep" updates when `value` hasn't changed.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test just moved below in the diff.

) {
const dispatcher = resolveDispatcher();
if (__DEV__) {
warning(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual change.

@sizebot
Copy link

sizebot commented Jan 30, 2019

React: size: 0.0%, gzip: 0.0%

Details of bundled changes.

Comparing: ba6477a...bd87a94

react

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react.development.js +0.5% +0.5% 98.24 KB 98.69 KB 25.6 KB 25.72 KB UMD_DEV
react.production.min.js 0.0% 0.0% 12.14 KB 12.14 KB 4.71 KB 4.71 KB UMD_PROD
react.profiling.min.js 0.0% 0.0% 14.3 KB 14.3 KB 5.24 KB 5.24 KB UMD_PROFILING
react.development.js +0.7% +0.8% 61.57 KB 62.02 KB 16.56 KB 16.7 KB NODE_DEV
React-dev.js +1.0% +0.9% 58.73 KB 59.33 KB 15.63 KB 15.78 KB FB_WWW_DEV
React-prod.js 🔺+0.1% 🔺+0.1% 15.36 KB 15.38 KB 4.1 KB 4.11 KB FB_WWW_PROD
React-profiling.js +0.1% +0.1% 15.36 KB 15.38 KB 4.1 KB 4.11 KB FB_WWW_PROFILING

Generated by 🚫 dangerJS

@Yurickh
Copy link
Contributor

Yurickh commented Jan 30, 2019

Did you meant useContext in the PR's title?

@gaearon gaearon changed the title Warn when second argument is passed to useCallback Warn when second argument is passed to useContext Jan 30, 2019
@gaearon gaearon merged commit 51c0791 into facebook:master Jan 31, 2019
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
@benwiley4000
Copy link

@gaearon Does this make the unstable_observedBits argument to useContext more unstable than the unstable_observedBits prop to createContext(...).Consumer? Is there a chance of either being removed in a React 16.x release?

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 6, 2019

Yes there’s always a chance of unstable APIs getting removed.

It doesn’t make it more unstable than the Consumer prop version. It’s just easier to pass by accident and get weird results. Which is why this one warns.

@benwiley4000
Copy link

Got it, thanks for the clarification!

@benwiley4000
Copy link

@gaearon So I have another question -- is this meant to stop anyone from using unstable_observedBits in a library? Often with unstable APIs it seems understood that library maintainers may use it, but with this warning anyone consuming my library and using a development build of React will get this warning over and over and they will probably freak out. I'm considering adding a hook export to a library I maintain, but I don't want to do that if I can't use unstable_observedBits The warnings in the console I'm seeing add way too much noise.

@windmaomao
Copy link

windmaomao commented Sep 1, 2021

The thing I see is that with this flag observedBits turned on, coupled with memo, a consumer can be bailed out now.

However I can't seem to get it working without a memo or a custom memoChild to wrap children under the provider.

import { memo } from 'react'

const equalWithoutChildren = (prev, next) => {
  for (let k in prev) {
    if (k === 'children') continue
    if (prev[k] !== next[k]) return false
  }
  return true
}

const memoChild = (Component) => {
  const Memo = memo(({ children, ...props }) => {
    return <Component children={children} {...props} />
  }, equalWithoutChildren)

  return Memo
}

export default memoChild

This is the way i use it,

  const Child = memoChild(Child_)
  <Provider value={...}>
    <Child />
  </Provider>

Please let me know if i'm doing anything wrong. Also is this useContextSelector about to make it targeted? I wonder if this issue is covered in that new proposal. Thank you team.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants