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

Feature request: have Context.Provider throw error if missing value prop #19020

Closed
heath-freenome opened this issue May 27, 2020 · 9 comments
Closed

Comments

@heath-freenome
Copy link

heath-freenome commented May 27, 2020

Just about every time I set up a new Context.Provider, I end up accidentally specifying a values prop rather than value. While it's a minor error, generally I build the container in which the provider resides and commit it to the code base before I ever use it. It's only later when I go to use it that I realize I did it again. Since the Context.Provider seems pretty much useless without a value prop specified, I'd love it if there was a prop error if it is missing... especially if another prop is defined on the Context.Provider instead.

@gaearon
Copy link
Collaborator

gaearon commented May 27, 2020

I think it does make sense to warn if !('value' in props). Want to send a PR?

@brunogonzales
Copy link

@heath-freenome I would love to take it if you don't mind, it's that okay?

@bvaughn
Copy link
Contributor

bvaughn commented May 28, 2020

This issue is all yours, @brunogonzales

I've added the "good first issue (taken)" label so that others will know not to start work on the issue. If you change your mind about the issue, no worries! Just let me know so that I can remove the label and free it up for someone else to claim.

Cheers!

@heath-freenome
Copy link
Author

@brunogonzales go for it! I wouldn't have time to get to it for a month at least. Please tag me on the PR as I would love to see the solution.

@charlie1404
Copy link
Contributor

@brunogonzales Have you started working on it ?? 😅
I have made changes locally to add warning and (added + updated) the tests but not really sure if that is the correct way to do that.
I am not sure if I am supposed to raise a pr if an issue is taken 😅 @bvaughn
but tried to do this, even when taken, because this seemed a bit easy to do with the limited knowledge I have 😓 and could get me to familiarize myself with this code.
If you have not started work on it or not planning to pick soon, shall I create PR with what I have done?
Else if you have started, please feel free to ignore, but please tag me on the PR, I would love to see the solution.

@brunogonzales
Copy link

@charlie1404 you can totally take this off my hands, I haven't started working on it yet. Please tag me on your PR as I would like to know the solution too.

@charlie1404
Copy link
Contributor

charlie1404 commented Jun 7, 2020

@bvaughn How to get reviewers added on pr ?
Is that automated/queued or, need to comment/tag someone on PR ??

@hemakshis
Copy link
Contributor

Hi! Is this issue still open and available? I would be happy to take it up if no one is working on it ATM.
Thanks

@eps1lon
Copy link
Collaborator

eps1lon commented Aug 21, 2020

Hi! Is this issue still open and available? I would be happy to take it up if no one is working on it ATM.
Thanks

This was fixed in #19054 and released in React 17 Realease Candidate. We just forgot to close this issue.

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

No branches or pull requests

7 participants