Integrated warning on returning null #6636
Conversation
Warns if user returns null to initialstate ( hopefully ). Things I don't know about this PR. -Integration with jest -If we should warn at all
ghost
added
| // This is probably bad practice. Consider warning here and | ||
| // deprecating this convenience. | ||
| warning( | ||
| typeof initialState === 'object' && initialState == null && !Array.isArray(initialState), |
There was a problem hiding this comment.
We already know that initialState === undefined so this condition will never be met and we'll always warn. It's also an entirely unactionable warning as this only happens when a component is mocked.
There was a problem hiding this comment.
Hmm, okay. Closing this then 😄
There was a problem hiding this comment.
I thought the comment was meant to warn if user returns null in initialState, (also forgot about first conditional). Is the comment meant to warn AND/OR remove the mocking function? As by now, there's only two places in the repository that uses ._isMockFunction
|
@ev1stensberg Hey, I've noticed you've been doing a few PRs lately, which is great. However a lot of them haven't made much progress. One of the things I'd like for us to do more of is discuss changes in behavior in an issue before implementing them. This allows discussion of the idea to happen and can shape & clarify the plan before any code gets changed. For example, filing an issue declaring your goal of warning when |
|
@zpao Yeah, was thinking the same thing when I made this PR. I'm working on implementing Also, question: Where's the roadmap for React? (Other than React future) If so, I doubt you've included all these little "issues" in the roadmap, which would be great for people that aren't necessary the best on filing quality PR's ( like me ). If so, it should include what each change/PR should include for each issue. What do you think? Also, thanks for the reminder, I know it's probably not in your best interest handling all these PR's. |
|
As of right now there is no specific roadmap. We might create a special repo for tracking it, but we haven’t committed to doing this yet. In the meantime, as @jimfb notes, good first bug is usually a good place to start. Please make sure to check that if you want to work on an issue, nobody else is already working on it, and leave comment stating you’d like to help, so nobody starts working on it right after you. For changes like this PR, it’s absolutely fine just to raise an issue with a suggestion first. This issue tracker is a good place for this. |
|
@gaearon @jimfb Yeah totally agree. Problem with issue tracker, is that the issue gets closed straight away without a good discussion that helps towards a complete solution. If its closed, it gets trashed and forgotten. Still waiting for someone to answer my closed thread on transferring state through props, but I know it won't be much of help when it's closed. No one is looking actively on closed threads. That's one of the reasons why I file PRs instead of issues. It has a higher response ratio than actual issues, when it should really be the other way around or at least much more balanced. |
Generally this doesn’t happen when we understand why you are trying to accomplish something, and it matches what we’d like to see in React. This issue is a good example of this at work: somebody saw a problem in React and explained it, the team agreed this is something they would like to see, and somebody else came along to implement it. However, issues like #6616 are harder to discuss because they lack the essential context:
When an issue fails to address these questions, keeping it open doesn’t help anyone because it is not actionable for the team or anyone else. On the other hand, focused, easy to understand issues like #6114 that don’t affect the public API are usually a better way to start contributing. I hope this helps! We mean well. |
|
#6616 actually refer to an existing issue, and the issue was built upon trying to move state in a more freely matter. And yeah, I agree to the bulletpoints, but I think that a discussion is meant to build up a case along the way too. For this example, it had the intent: feedback and reiteration And I don't really see any other way to get people to be notified about this other than the issue tracker as a informative source. Social media / stack overflow are not good streams for that kind of content, you won't guarantee to get feedback from authors. To conclude: Feedback is gold, but in order to make a strong PR and use case, you need to have a discussion ---> of which a closed issue won't contain. Issue trackers should be used for reiteration, and I think that these bullet points are not always relevant, you need to find out as a group / community how you want to approach rather than just as individual thoughts, you'll get a better end-product. About the last line -- Yeah you guys are awesome, so I'm taking this as constructive criticism 💙 |
|
Forgot to mention that you got React specific forums, but with a proposal-ish, you'll need a place to host your code/thoughts, rather than a gist. What I think we should have, is a label named "forum discussion" or "feature discussion" so people actually see the "issue" , as by now I think we only have the discussion label on GH. |
Warns if user returns null to initialstate ( hopefully )
Things I don't know about this PR.
Feel free to close this PR if this change is not appropriate( @jimfb ) 😆