-
Notifications
You must be signed in to change notification settings - Fork 48.6k
Warn if context values differ, related to issue #2 #2602
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
Conversation
@@ -595,18 +595,39 @@ var ReactCompositeComponentMixin = assign({}, | |||
* TODO: Remove this check when owner-context is removed | |||
*/ | |||
_warnIfContextsDiffer: function(ownerBasedContext, parentBasedContext) { | |||
if (!__DEV__) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if the code gets properly stripped out for the minified build if you don't do if (__DEV__) { ... }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(It does in open-source, not sure for www though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it works internally (basically __DEV__
gets replaced with 0
from what I can find)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I meant the minifier specifically – I'm sure the runtime behavior is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I was talking about actually, we replace before minifying.
But that said, we don't actually need this check and we should just wrap callsites with if (__DEV__)
. The only callsite is already wrapped so it's unnecessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, great, removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I think I'm still talking past you – my understanding is that your minifier is smart enough to turn if (0) { foo(); }
into nothing but isn't smart enough to turn if (1) { return; } foo();
into nothing.
1abe1dd
to
84f2700
Compare
84f2700
to
90d75ab
Compare
Warn if context values differ, related to issue #2
Warn if context values differ, related to issue #2112