-
-
Notifications
You must be signed in to change notification settings - Fork 389
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
v7 and v8 are not tree-shakable #347
Comments
Sorry, but at the moment we are not going to release the package with esm, see expressjs/discussions#323 |
I thought @blakeembrey said he was planning on supporting a dual mode package? Maybe I misunderstood our discussion yesterday, but I think we might want to leave issues like this open for discussion to ensure we get feedback and have time to understand the issues folks are reporting. In this case, I would love to know what benefit you get out of treeshaking a single file dependency? The file is 14kb unminified, and nearly all of it would be used together in all the use cases I have seen. So what is the end user benefit of treeshaking for such a package? |
Also, CJS tree-shakes just fine (albeit often with an extra plugin) |
To not ship the entire file to the client and avoid import errors of users who are using Paraglide JS.
I am left with not adopting path-to-regexp, forking it, or using something else. Which is fine, dependent on the goal that the express committee defines |
What does it result in if you strip out all but what you need? (EDIT for clarity, or do you mean that it is 1kb with the old version of
What CJS issues? I am not sure I follow this. CJS has not had "issues", so I am not clear on what this means.
@samuelstroschein did you explore this? And if so, can you show that tree-shaking the CJS still results in a worse bundle? |
@wesleytodd thanks for following up.
Issues like these, #346, are common. For example, ESM exports missing in the package.json and bundlers can start throwing errors if moduleResolution: node16 is set. As library author, I am not maintaining CJS related issues anymore. Modern projects use ESM, and legacy projects are adding ESM support.
The ESM that only imports Have you explored emitting all dependencies as ESM and only keeping the source code of Express JS as CJS? That might merge the best of both worlds. Node can now natively import ESM from CJS (in v23 and above). You could make Express 6 users use Node >23 or higher while the ecosystem can leave the CJS baggage behind (especially if the source code is already written in ESM) |
CJS isn't baggage and isn't going away, and a bundler that doesn't support it is a broken one. Which bundler are you using that can't treeshake CJS? |
The video on the reproduction took long to upload. Here it is https://www.loom.com/share/6a2d5abffd1d48f3b06526779eab85b5?sid=c0299037-a6e8-4a02-ae6d-c3bff3b79404 No bundler I am aware of can efficiently tree-shake CJS. The bundler of questions in the video is esbuild, which is used by Vite. In any case, I am looping myself out of this discussion. I presented my arguments in favor of letting dependencies decide if they dual export CJS or ESM. As the library author of Paraglide JS, I wouldn't adopt a CJS dependency. I invested too much time into debugging CJS in the past. Figuring out which bundler works and which does not, or why a user's project setup is broken, is exactly what I don't want to do. |
@samuelstroschein I used yesterday's meeting to discuss ESM in our packages and there was buy-in, so I'm reopening the issue. It was closed prematurely. Agree with everything you've brought up and unfortunately it'll be a major version to add ESM so give me some time to investigate if there's any other breaking-ish changes that might be rolled into a new release of the package with ESM. |
Sounds great. I will bundle path to regex manually in the meantime. For what it's worth, it seems like Node 20 will get require(esm) back-ported in March. If node 20 gets require(esm), there is no reason to ship CJS at all anymore. You could go all in ESM. Might also make the CJS vs ESM discussion in express js redundant. CJS will not be a requirement in a month anymore. You could ship all dependencies, and express js itself, as ESM. |
People take years to upgrade; it'll take many years after require(esm) is in all LTS lines before that's a viable option. It's coming, though! |
@ljharb are you sure? Node 20 is the only LTS version from May 2025 onwards. If node 20 gets require(esm), April 2025 (in 2 months!) is the last month for "necessary" support of CJS to support Node LTS versions. It seems reasonable to release a new major version of dependencies that drops CJS in favor of ESM now/April, no? |
The metrics say that @ljharb is correct historically. It takes a few years for the install counts and download counts to shift after the maintenance period is over most of the time. Either way, that is not something we need to bike shed or worry about here because the current |
Problem
Tree-shaking is not working with v7 and above because the ESM build has been removed.
I am the author of Paraglide JS. Tree-shaking is a key differentiator. For locale detection reasons, I'd like to use path-to-regexp on the client. The lack of tree-shaking (which would increase the runtime size from <1kb to >4kb) and ESM problems like #346 prevent me from adopting path-to-regexp.
Using
URLPattern
instead of path-to-regexp is not possible because a reverse compile function is missing whatwg/urlpattern#73.Reproduction
build
To inspect the bundle, disable
minify: true
in the settings. V6 only contains functions used incompile,
while >V7 bundles everything.Proposal
Re-introduce dual CJS + ESM build, or say goodbye to CJS altogether, now that Node supports native
require(esm)
.The text was updated successfully, but these errors were encountered: