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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added warning to <Context.Provider> in case no value prop is provided #19054

Conversation

@charlie1404
Copy link
Contributor

@charlie1404 charlie1404 commented May 30, 2020

Summary

This PR aims to add a fix for #19020

  1. There is a duplication in what I have done in files
    • packages/react-reconciler/src/ReactFiberBeginWork.new.js
    • packages/react-reconciler/src/ReactFiberBeginWork.old.js

In feature flag saw const enableNewReconciler = false but still cannot jump to any solid conclusions hence added in both.

Test Added
packages/react-reconciler/src/__tests__/ReactNewContext-test.js test to detect warning.

Tests Updated Reason
As now <Context.Provider> now throws a warning if value prop is not given, and hence updated the ones which were not having value prop to have null now.

Points to be discussed

  1. What should be the warning message, (not really good with this) 馃槄
  2. Should this be included in both old and new, or I have added in the totally wrong place.
    • if correct, out of curiosity why there are 2 files, seems almost identical, and only these but a whole lot files are in the same format. (please ignore if very long explanation and out of the scope of this PR)
  3. Should any more test cases to be added in this?

cc @brunogonzales @heath-freenome

@codesandbox
Copy link

@codesandbox codesandbox bot commented May 30, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1603910:

Sandbox Source
cool-leftpad-pem6m Configuration
@sizebot
Copy link

@sizebot sizebot commented May 30, 2020

Warnings
鈿狅笍 Could not find build artifacts for base commit: 1d85bb3

Size changes (stable)

Generated by 馃毇 dangerJS against 1603910

@sizebot
Copy link

@sizebot sizebot commented May 30, 2020

Warnings
鈿狅笍 Could not find build artifacts for base commit: 1d85bb3

Size changes (experimental)

Generated by 馃毇 dangerJS against 1603910

@charlie1404
Copy link
Contributor Author

@charlie1404 charlie1404 commented May 30, 2020

did like the name of the code sandbox 馃ぃ.
Edit:
code sandbox bot edited the old message

old new
cocky-margulis-zss43 unruffled-bardeen-vj2s6

wondering why cannot just update deps it old one 馃槥

@heath-freenome
Copy link

@heath-freenome heath-freenome commented Jun 3, 2020

Question, would making the value prop .isRequired also accomplish this?

@charlie1404
Copy link
Contributor Author

@charlie1404 charlie1404 commented Jun 4, 2020

Question, would making the value prop .isRequired also accomplish this?

@heath-freenome
Initially gave a thought on that idea, but did not proceed for 2 reasons:

  1. If we use prop types there is a possibility that value prop can actually be null or undefined, say if the value is being set from some key in state, which is an object, but the key used is null or undefined for some reason, in that case, it will start adding warnings to console, not sure if we should do that, worth discussing. The 'value' in props only gives a warning if there is no value prop supplied to <Context.Provider>.

  2. Now if we use prop types I had to do Object.assign with workInProgress.type.propTypes, but as this is an internal component and had no prop types defined hence was undefined, not sure if I should add a completely new object, ignoring old one or still do Object.assign maybe I was looking at the wrong place.

But pointing to prop types gave me a better message, thanks for that, updating it now.

@eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jun 4, 2020

React just recently removed all runtime dependencies to prop-types. It's unlikely they re-introduce it. Especially given prop-types cannot distinguish between a missing prop and an undefined prop which @charlie1404 already pointed out.

if (!hasWarnedAboutUsingNoValuePropOnContextProvider) {
hasWarnedAboutUsingNoValuePropOnContextProvider = true;
console.error(
'The prop `value` is required in `Context.Provider`, have you misspelled it',

This comment has been minimized.

@gaearon

gaearon Jun 30, 2020
Member

Let's reword this a bit:

The `value` prop is required for the `<Context.Provider>`. Did you misspell it or forget to pass it?

This comment has been minimized.

@charlie1404

charlie1404 Jun 30, 2020
Author Contributor

Thanks for the message 馃槄 .

This comment has been minimized.

@charlie1404

charlie1404 Jun 30, 2020
Author Contributor

updated

if (!hasWarnedAboutUsingNoValuePropOnContextProvider) {
hasWarnedAboutUsingNoValuePropOnContextProvider = true;
console.error(
'The prop `value` is required in `Context.Provider`, have you misspelled it',

This comment has been minimized.

@gaearon

gaearon Jun 30, 2020
Member

Same

@@ -263,13 +263,13 @@ describe('ReactContextValidator', () => {

class Component extends React.Component {
render() {
return <TestContext.Provider />;
return <TestContext.Provider value={null} />;

This comment has been minimized.

@gaearon

gaearon Jun 30, 2020
Member

Make this value={undefined} instead and you won't need to change the message.

This comment has been minimized.

@charlie1404

charlie1404 Jun 30, 2020
Author Contributor

now using undefined and reverted message

Copy link
Member

@gaearon gaearon left a comment

Small nits and this is good to go.

@gaearon gaearon merged commit f4097c1 into facebook:master Jun 30, 2020
32 checks passed
32 checks passed
Facebook CLA Check Contributor License Agreement is valid!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_dom_fixtures Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_persistent Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: RELEASE_CHANNEL_stable_yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts Your tests passed on CircleCI!
Details
ci/circleci: process_artifacts_experimental Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: sizebot_experimental Your tests passed on CircleCI!
Details
ci/circleci: sizebot_stable Your tests passed on CircleCI!
Details
ci/circleci: yarn_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_flow Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint Your tests passed on CircleCI!
Details
ci/circleci: yarn_lint_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_devtools Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_build_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_prod_www_variant Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www Your tests passed on CircleCI!
Details
ci/circleci: yarn_test_www_variant Your tests passed on CircleCI!
Details
ci/codesandbox Building packages succeeded.
Details
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 30, 2020

Coolio, thanks.

@charlie1404 charlie1404 deleted the charlie1404:19020-warn-if-no-value-prop-in-context-provider branch Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can鈥檛 perform that action at this time.