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

add process guard #777

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add process guard #777

wants to merge 1 commit into from

Conversation

@koba04
Copy link
Member

koba04 commented Jan 6, 2022

@guybedford
Thank you for your work!
I think process.env.NODE_ENV is intended to be replaced with the actual value of NODE_ENV in build time. If we add a guard condition like this PR, this would become a problem because typeof process !== 'undefined' is always false in runtime.

// When NODE_ENV is "development"
typeof process !== 'undefined' && process.env.NODE_ENV !== 'production'
// This becomes like the following in runtime
typeof process !== 'undefined' && "development" !== 'production'
// process is undefined in runtime, so this condition is always `false`

But, I understand your concern, so we should find a way to work with JSPM.

@guybedford
Copy link
Author

@koba04 process is a Node.js global variable - not a browser one. So the pattern of assuming it exists in browser modules means the browser modules being written are not real browser modules (https://twitter.com/guybedford/status/1474434054590107649?s=20).

Build tools that handle the replacement should know to intelligently replace the typeof check as well. I haven't tested recently, but I believe Webpack does this and it's maybe just RollupJS that doesn't?

Perhaps we can follow up with build tool issues further as this is important to fix.

@koba04
Copy link
Member

koba04 commented Jan 7, 2022

Personally, I'd like to remove prop-types from its dependencies because it's no longer a popular way of checking props. But our API documentation in the website depends on propTypes, so we have to remove the dependency before removing it.

I noticed that prop-types is used in other places without the guard condition, such as https://github.com/reactjs/react-transition-group/blob/master/src/Transition.js, so I'm curious why the guard condition is only required here. Could we remove the condition? It would be nice if we could remove the condition without adding the guard condition.

@guybedford
Copy link
Author

Checking src/Transition.js it looks like that does import the src/utils/PropTypes.js file, so does effectively depend on the guard as well.

Agreed fully removing would be nice, was just posting the easiest fix to start.

I will aim to follow up build issues if that's a concern.

@koba04
Copy link
Member

koba04 commented Jan 8, 2022

@guybedford I've noticed that there are other process.env.NODE_ENV in the build files. react-transition-group uses babel-plugin-transform-react-remove-prop-types, which is a babel plugin to remove PropTypes definitions from production builds. It inserts process.env.NODE_ENV !== "production" into assignments of propTypes with the wrap option. https://www.npmjs.com/package/babel-plugin-transform-react-remove-prop-types

If we keep using PropTypes, we have to remove the babel plugin as well.

@koba04
Copy link
Member

koba04 commented Jan 8, 2022

I'd like to take an option to remove PropTypes to fix this issue.

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

Successfully merging this pull request may close these issues.

2 participants