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

Remove "Failed Composite propType" checking from warning #153

Closed
wants to merge 1 commit into from
Closed

Remove "Failed Composite propType" checking from warning #153

wants to merge 1 commit into from

Conversation

keyz
Copy link
Contributor

@keyz keyz commented May 23, 2016

Fixes #152. facebook/react#6824 removed duplicate propType checking, therefore the blacklist at https://github.com/facebook/fbjs/blob/master/src/__forks__/warning.js#L34-L36 is no longer necessary.

@sophiebits
Copy link
Contributor

sophiebits commented May 23, 2016

Thanks, looks good. @zpao This should go in the next "major" (0.9.x, not 0.8.x) release – can we merge now or should we hold off?

@zpao
Copy link
Member

zpao commented May 25, 2016

Let's hold off. I'm going to take #151 and probably call 0.8 done then unless something pressing comes up, but we could branch for that if we need.

@amireh
Copy link

amireh commented May 27, 2016

Hate to hijack, but is there any chance we can blacklist the contentEditable warning we get when using Draft.js?

It reads something like this:

A component is `contentEditable` and contains `children` managed by React. It is now your 
responsibility to guarantee that none of those nodes are unexpectedly modified or duplicated. 
This is probably not intentional.

@sophiebits
Copy link
Contributor

If you're on the latest version of React and Draft you shouldn't be seeing that warning.

@amireh
Copy link

amireh commented May 27, 2016

Hmm, we just barely made the 0.13 to 0.14 upgrade and I don't think the upgrade to 0.15 will happen anytime soon. Would you accept a PR for such a blacklist if I made one?

(I would've personally forked the package and avoided annoying upstream if it was a peerDependency of Draft/React but sadly it's not so that option isn't available to us.)

@zpao
Copy link
Member

zpao commented May 27, 2016

No, we wouldn't accept a PR for that, sorry. We'd have to do another React 14 release to pick it up and it would involve branching here and it's ultimately not worth the effort and burden for a minuscule percentage of users. The path is to upgrade React (which should be pretty painless ultimately but I can understand not rushing into that).

@sophiebits
Copy link
Contributor

The React 15 upgrade has very few breaking changes so I hope you can upgrade soon.

@zpao
Copy link
Member

zpao commented Jul 11, 2016

Would it matter much if we removed this in 0.8.4? What's the impact on people who haven't upgrade to React 15.2?

@sophiebits
Copy link
Contributor

Double prop types warnings for everything.

@zpao
Copy link
Member

zpao commented Jul 11, 2016

We're not inadvertently doing the inverse and hiding the only warnings people would see (for people who have upgraded)?

@sophiebits
Copy link
Contributor

I don't think so? Context: For years now, we ran prop types checks at element creation "Failed propType:" and at component mount "Failed Composite propType:". We filtered out the former at FB where we had a big legacy codebase, and filtered out the latter in open source with this check in fbjs. I recently cleaned up our internal callsites so we were able to switch to element-time validation everywhere, and filtering out "Failed Composite propType:" both at FB and in open source. Now no one was looking at those messages ever, so @keyanzhang removed the code that produced them – and we can remove the filter since it's not doing anything when used in React 15.2+.

@zpao
Copy link
Member

zpao commented Jul 11, 2016

Oh gotcha. So current code is dead for 15.2 (well it's running and slowing down every warning call).

Let's just pull this into the next release then (soonish, will mostly just be #150) and then we can bump the dep in React for next version.

@ghost ghost added the CLA Signed label Jul 12, 2016
zpao pushed a commit to zpao/fbjs that referenced this pull request Jul 18, 2016
@ghost ghost added the CLA Signed label Jul 18, 2016
@zpao
Copy link
Member

zpao commented Jul 18, 2016

Pushed in 4c61489.

@zpao zpao closed this Jul 18, 2016
zpao pushed a commit that referenced this pull request Jul 18, 2016
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.

4 participants