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

v7 and v8 are not tree-shakable #347

Open
samuelstroschein opened this issue Feb 3, 2025 · 13 comments
Open

v7 and v8 are not tree-shakable #347

samuelstroschein opened this issue Feb 3, 2025 · 13 comments

Comments

@samuelstroschein
Copy link

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

  1. Open https://bundlejs.com/?q=path-to-regexp%407&treeshake=%5B%7B+compile+%6D%5D
  2. Press build
  3. Repeat with v7 and v8

To inspect the bundle, disable minify: true in the settings. V6 only contains functions used in compile, while >V7 bundles everything.

Proposal

Re-introduce dual CJS + ESM build, or say goodbye to CJS altogether, now that Node supports native require(esm).

@bjohansebas
Copy link

Sorry, but at the moment we are not going to release the package with esm, see expressjs/discussions#323

@bjohansebas bjohansebas closed this as not planned Won't fix, can't repro, duplicate, stale Feb 3, 2025
@wesleytodd
Copy link
Member

wesleytodd commented Feb 4, 2025

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?

@ljharb
Copy link

ljharb commented Feb 4, 2025

Also, CJS tree-shakes just fine (albeit often with an extra plugin)

@samuelstroschein
Copy link
Author

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

To not ship the entire file to the client and avoid import errors of users who are using Paraglide JS.

  1. Paraglide ships approx 1KB right now, bundling this dependency yields 4KB gzipped despite only needing match and compile
  2. Under no circumstances do I want to maintain CJS issues (the inverse of your desire of only CJS for express to avoid ESM efforts).

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

@wesleytodd
Copy link
Member

wesleytodd commented Feb 4, 2025

Paraglide ships approx 1KB right now, bundling this dependency yields 4KB gzipped despite only needing match and compile

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 path-to-regexp bundled? that was unclear in your comment)

Under no circumstances do I want to maintain CJS issues

What CJS issues? I am not sure I follow this. CJS has not had "issues", so I am not clear on what this means.

Also, CJS tree-shakes just fine (albeit often with an extra plugin)

@samuelstroschein did you explore this? And if so, can you show that tree-shaking the CJS still results in a worse bundle?

@samuelstroschein
Copy link
Author

samuelstroschein commented Feb 4, 2025

@wesleytodd thanks for following up.

What CJS issues? I am not sure I follow this. CJS has not had "issues", so I am not clear on what this means.

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.

What does it result in if you strip out all but what you need

The ESM that only imports compile and match is 1.84kb minified and gzipped vs. 2.4kb for the CJS build.


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)

@ljharb
Copy link

ljharb commented Feb 4, 2025

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?

@samuelstroschein
Copy link
Author

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.

@blakeembrey
Copy link
Member

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

@blakeembrey blakeembrey reopened this Feb 4, 2025
@samuelstroschein
Copy link
Author

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.

@ljharb
Copy link

ljharb commented Feb 6, 2025

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!

@samuelstroschein
Copy link
Author

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

@wesleytodd
Copy link
Member

wesleytodd commented Feb 6, 2025

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 path-to-regexp will receive support (like the 0.x line does) because it is shipped with express and we have a multi year support plan for that. Additionally, for important packages like this we can partner with our friends 😉 to get never ending support for those who need it to fix security issues that may crop up. So lets not worry too much about it for this case.

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

5 participants