npm package contains minified files
joepie91 opened this issue · 13 comments
Is your feature request related to a problem? Please describe.
The package as it is published on npm currently contains multiple minified files; this is problematic from a security auditing perspective, as minified code is difficult to audit. This is particularly problematic because of the often security-sensitive nature of the uuid module.
A more detailed explanation of the problem with minified builds and why they are unnecessary to publish in the first place, can be found here.
Describe the solution you'd like
Removing all minified builds from the npm package in a future release, instead either a) providing instructions for users on how to obtain a minified build, or b) if necessary for certain CDNs, publishing the minified builds in a separate package that users wouldn't install into their project.
Describe alternatives you've considered
- Reproducing from source: Requires auditing the entire build stack due to potential supply chain attacks, with build tools being able to modify the code going through them.
- Prettifying minified code and auditing that: Cumbersome, unreliable because a lack of meaningful names makes it hard to understand the code correctly.
- Just not using the minified files: For a reliable dependency audit, all files present in the dependency tree must be audited, because there is (AFAIK) no runtime-level mechanism to block certain files from being loaded in eg. a multi-stage attack.
Additional context
N/A
Is your issue with the existence of multiple builds, or that [some of] those builds are minified?
The simplest solution (for us) would be to just turn off the compact flag in rollup.config.js. Keeps the existing package structure (probably won't break anything) while disabling minification.
While multiple builds do add auditing overhead and so are ideally avoided, it's not a huge issue, because most of the time you can simply diff between the builds and verify that the only differences are syntactic or in common boilerplate. The real problem lies with the minification - which is hard to read, and usually neither efficiently diffable against other builds nor against previous versions.
So I'd say that simply disabling minification would address my concern sufficiently well, yeah :)
So essentially you want to get rid of the minified files in this folder: https://unpkg.com/browse/uuid@8.3.2/dist/umd/ ?
I think those UMD builds are really mostly a legacy from a time (~10 years ago) when bundlers were not the default for browser code.
I would be surprised if they are being used extensively these days outside of e.g. educational tools like http://repl.it and even those tools have good support for easily installing npm dependencies these days.
So I would actually vote for entirely removing the umd build in the next major version. We could still consider publishing them as a separate package, e.g. uuid-umd.
So adding this to the next major version milestone. Happy to accept pull requests as well :)
Regarding multiple builds: I believe as long as we have both CommonJS and ESM in heavy use across browsers and server environments we won't be able to get rid of them.
So I would actually vote for entirely removing the umd build in the next major version.
My vote goes towards this as well 👍
Regarding multiple builds: I believe as long as we have both CommonJS and ESM in heavy use across browsers and server environments we won't be able to get rid of them.
Personally, I would prefer to just rip the band-aid off and only ship ESM. It's supported by all supported versions of Node.js, and I believe that all major build tools for the browsers supports it, although I'm not 100% on that.
If we still need to support CommonJS I think that we should use a wrapping approach so that we don't need to include two builds for Node.js. This also has the upside that if you have multiple dependencies that are using uuid, and some have upgraded to ESM but some of them is still on CommonJS, then they will still only load one copy of the uuid module, instead of loading two separate copies.
Today we are actually shipping three versions for Node.js:
dist/esm-node/index.jswhich contains ESM syntax source codedist/index.jswhich contains CommonJS syntax source codewrapper.mjswhich imports the CommonJS code, and exports it as an ESM module
I think we could remove dist/esm-node and only use the wrapper?
Circling back to ESM only though, I think that my preference would be to only ship two files to Npm: browser.js and node.js, both produced by Rollup (but not minified, just rolled up into one file, to simplify the Node vs. Browsers story) 🤔
All in all I think this is a great discussion to have!
I would be fine with either disabling minification or removing the minified build entirely, I don't have a strong preference. Either way the minified code would no longer be in the package :)
That having been said, however...
Personally, I would prefer to just rip the band-aid off and only ship ESM. It's supported by all supported versions of Node.js, and I believe that all major build tools for the browsers supports it, although I'm not 100% on that.
This is a really bad idea. Even leaving aside the problems with ESM as a module system (which are one of the reasons why "just migrate your code to ESM" is not a viable answer), ESM cannot be imported from CJS while it is possible the other way around.
So the real-world consequence of making the package ESM-only would be to essentially lock out most of the ecosystem (including breaking dependent trees), cause major headaches for nearly everybody using the module, and likely cause version stragglers like is happening with Sindre's packages. All the while there's really not any benefits to doing this in the first place.
If we still need to support CommonJS I think that we should use a wrapping approach so that we don't need to include two builds for Node.js.
If a wrapping approach is viable (I haven't checked the latest situation on this), then that would work, as it wouldn't break imports from CJS. The important thing here would be to not needlessly break compatibility, IMO.
This is a really bad idea. Even leaving aside the problems with ESM as a module system (which are one of the reasons why "just migrate your code to ESM" is not a viable answer), ESM cannot be imported from CJS while it is possible the other way around.
I didn't really find any critique of ESM, just that CommonJS also can do everything that ESM can do. That might very well be true, and I actually used to agree that we shouldn't move away from CommonJS. But at this point that ship has sailed and I don't see anyone rolling back the change to ESM...
Anyhow, I don't see a rush in doing this for uuid, and I don't think that we should do it just because. Only if it makes it easier to maintain the package...
If a wrapping approach is viable (I haven't checked the latest situation on this), then that would work, as it wouldn't break imports from CJS. The important thing here would be to not needlessly break compatibility, IMO.
👍
This is a really bad idea.
I actually disagree with most of what you wrote there but the time for debating this has passed. ESM is the way forward. The uuid source has been ESM-based for... what... two years now? All we're talking about here is whether or not to remove the UMD build that is automatically generated by rollup.
One thing I do feel pretty strongly about is that module owners shouldn't have to be in the business of packaging code for different environments. The fact we provide different builds for node/browser/ESM/CJS is a testament to @ctavan's passion for making sure this stuff works on every conceivable platform.
To that end, I'd be fine taking a stance that we do ESM only, and if anyone wants something else, they need to figure out a toolchain that works for them.
Wow, that thread escalated quickly 😂
While I would love to ease our pain as maintainers of this package, I think the uuid package has a long tradition of providing a good deal of backwards compatibility and I think this has actually helped keep the amount of noise on this repo relatively low since it mostly just works for most people…
I'd be afraid that restricting ourselves to only ESM would actually cause a lot of pain in the community, even if that may be the most modern solution at the moment.
So when we look at the support matrix right now we have:
| CommonJS | ESM | |
|---|---|---|
| NodeJs | + | + |
| Browser | - | + |
In practice the discussions have recently gone even in the opposite direction of considering to add a browser CommonJS build, see #616 (comment) (<- that comment contains 4 more references where folks have been requesting this).
I'm still convinced that adding a CommonJS browser build would be fundamentally wrong at this point in time. But I equally think that it's not realistic to get rid of the CommonJS Node build.
BTW to elaborate a bit on the export section in:
Lines 21 to 29 in 3a033f6
node.moduleis a pure Node.js ESM build which only gets chosen when people create bundles for node.js (e.g. with webpack or rollup). Yes, apparently people do that for serverless environments 🤷♂️ .node.requiregets loaded when yourequire('uuid')in Node.jsnode.importgets loaded when youimport 'uuid'in Node.js and this is actually a wrapper around the CommonJS module. This is a mitigation to the dual package hazard to ensure that only one instance of UUID gets instantiated even if in your dependency tree some modules areimporting and some arerequire()ing uuid.defaultis a browser ESM build that gets picked up by bundlers when building for the browser (we use ESM here to support proper tree shaking).
Sometimes people manage to misconfigure their bundlers so that the bundler is looking for a CommonJS build which we don't provide. I think this is always the wrong thing to do when using a bundler.
To be clear, the only way a wrapping approach is viable for any package is if everything is in CJS, with maybe a thin ESM wrapper around it.
If the desire is to avoid a build process while not excluding the majority of the ecosystem (for evidence of this, look at any popular package that's gone ESM-only, and see how tiny a percentage of its users uses those versions), authoring in CJS directly is the only way to do that.
@ctavan the only tweak to your exports field is that if you want to also have your package work in node v13.0-13.6, you'd make the dot object be an array of "object" and "default filepath string".
Thanks for chiming in @ljharb!
To be clear, the only way a wrapping approach is viable for any package is if everything is in CJS, with maybe a thin ESM wrapper around it.
Yes, that's what we're doing here for Node.js!
If the desire is to avoid a build process while not excluding the majority of the ecosystem (for evidence of this, look at any popular package that's gone ESM-only, and see how tiny a percentage of its users uses those versions), authoring in CJS directly is the only way to do that.
Agree! But since we want to provide an independent build that also makes sense for browser bundlers, ESM does have advantages for those environments (even if webpack managed to treeshake the CommonJS version of this module as well). I'm fine with keeping a build step, now that we have it.
@ctavan the only tweak to your exports field is that if you want to also have your package work in node v13.0-13.6, you'd make the dot object be an array of "object" and "default filepath string".
I was considering that back when I introduced pkg.exports but decided against supporting these intermediate non-LTS Node.js versions. When they were still current, a few folks were complaining, but "upgrade to the next LTS release" was a viable solution for all of them, it was usually just an oversight that they were still on a 13.x version.
considering that the "default" key should work in node 13.3-13.6, and it's only v13.0-13.2 that will be completely unable to use the package, you're probably fine (altho in my packages i test on these versions too, just in case)