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

Shouldn't loose-envify be in devDependencies instead of dependencies? #203

Closed
autra opened this issue Aug 8, 2018 · 7 comments
Closed

Comments

@autra
Copy link
Contributor

autra commented Aug 8, 2018

If I'm not mistaken, it's only used in the umd script in the package.json. If I understand correctly, this script builds a bundle for browsers, and will only be used in dev process.

This would remove unused transitive dependencies when using a package that depends on prop-types.

@ljharb
Copy link
Collaborator

ljharb commented Aug 8, 2018

Anything that’s needed in production, even conceptually, is a runtime dep - so the fact that it’s included in a bundle doesn’t mean it shouldn’t be installed as a dep.

@autra
Copy link
Contributor Author

autra commented Aug 8, 2018

Anything that’s needed in production, even conceptually, is a runtime dep

Of course, but it's not the case for loose-envify here, as it is used only by browserify (a dev deps itself btw). Or did I miss one usage?

But I'm not sure of what you mean:

the fact that it’s included in a bundle doesn’t mean it shouldn’t be installed as a dep.

you mean "the fact that it's not included in a bundle doesn't...", right? Just to be sure I understood correctly.

@ljharb
Copy link
Collaborator

ljharb commented Aug 8, 2018

ahhh gotcha, i misunderstood. Yes, if loose-envify is part of the build process then it should definitely be a dev dep.

@billyjanitsch
Copy link

@ljharb @autra the reason it was listed as a dependency rather than a dev dependency is because these lines tell the app level browserify to use loose-envify when processing the package (to remove the process.env.NODE_ENV switches). This doesn't happen at package build time because those switches need to be left in the published package. Many other packages, such as react-router, invariant, and react itself, have loose-envify as a production dependency for the same reason.

So prop-types@15.7.0 likely broke support for browserify. It would probably be better to do this in a major release, accompanied by the removal of the package.json lines that I linked.

@ljharb
Copy link
Collaborator

ljharb commented Feb 11, 2019

@billyjanitsch if that’s the case, we can revert it and put it back - can you help me by providing a quick test case to verify that it does indeed need to be a runtime dep?

@ljharb ljharb reopened this Feb 11, 2019
@autra
Copy link
Contributor Author

autra commented Feb 12, 2019

So prop-types@15.7.0 likely broke support for browserify. It would probably be better to do this in a major release, accompanied by the removal of the package.json lines that I linked.

Sorry for that! I didn't intent to remove browserify support though, so maybe it's better to simply revert it? I see no other way if we want to keep this support.

@ljharb ljharb closed this as completed in 32114d3 Feb 12, 2019
@ljharb
Copy link
Collaborator

ljharb commented Feb 13, 2019

v15.7.2 is released; please file an issue if you see any problems!

@ljharb ljharb mentioned this issue Apr 11, 2019
2 tasks
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

No branches or pull requests

3 participants