search

facebook / prop-types

Runtime type checking for React props and similar objects
MIT License
4.48k stars 356 forks source link

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

Closed autra closed 5 years ago

autra commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

billyjanitsch commented 5 years ago

@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 commented 5 years ago

@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?

autra commented 5 years ago

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 commented 5 years ago

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