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

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-ci
Copy link

codesandbox-ci 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 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 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 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

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

@charlie1404
Copy link
Contributor Author

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 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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's reword this a bit:

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the message 😅 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

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

Choose a reason for hiding this comment

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

Same

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

now using undefined and reverted message

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Small nits and this is good to go.

@gaearon gaearon merged commit f4097c1 into facebook:master Jun 30, 2020
@gaearon
Copy link
Collaborator

gaearon commented Jun 30, 2020

Coolio, thanks.

This was referenced Mar 15, 2021
@mezzab
Copy link

mezzab commented Oct 18, 2022

What if I don't want to send a value prop to my Context. I want to use my defaultValue instead of setting a new value to my context.

For example:

export const LoggerContext = createContext(new FrontendLogger(getLogger(providerId)));


// component
...
return (<LoggerContext.Provider>{renderModal()}<LoggerContext.Provider>)

This is throwing me the error:
Property 'value' is missing in type '{ children: Element; }' but required in type 'ProviderProps'

Can we make it optional?
cc @gaearon

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.

None yet

7 participants