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
Optimize #268
Optimize #268
Conversation
@noscripter NODE_ENV is replaced statically with bundler for browsers. It should not be extracted. |
Thanks for that info, can you provide more detail for me to dive into? |
This convention is shared across many packages (especially react ones) |
Coool, thanks. |
@@ -206,7 +206,8 @@ module.exports = function(isValidElement, throwOnDirectAccess) { | |||
} | |||
} | |||
} | |||
if (props[propName] == null) { | |||
// undefined == null and null == null are true | |||
if (props[propName] === null || props[propName] === undefined) { |
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.
why is this a better change? == null
is more reliable, since undefined
can be redefined.
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.
why is this a better change?
== null
is more reliable, sinceundefined
can be redefined.
Thanks @ljharb I found this question on stackoverflow, it looks like a problem related to legacy browsers only, but your review did make sense.
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.
Also according to MDN, undefined should be un-writable but it's not a reserved word, so in some context it may be redefined as well.
(function() { var undefined = 'foo'; console.log(undefined, typeof undefined); })();
@@ -47,6 +46,7 @@ | |||
"eslint": "^5.13.0", | |||
"in-publish": "^2.0.0", | |||
"jest": "^19.0.2", | |||
"loose-envify": "^1.4.0", |
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.
this must be a runtime dependency, otherwise #203 (comment) breaks people.
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.
this must be a runtime dependency, otherwise #203 (comment) breaks people.
Got it, I thought it was only used for build in the first place.
[x] extract process.env.NODE_ENV