Reintroduce ESM build with CJS compatibility#439
Reintroduce ESM build with CJS compatibility#439wojtekmaj wants to merge 3 commits intopillarjs:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Reintroduces an ESM distribution for path-to-regexp while keeping the existing CommonJS entrypoint working for current consumers.
Changes:
- Switches package metadata to dual ESM/CJS via conditional
exports, plusmodulefield. - Replaces the build pipeline with
tsdownto emit both CJS and ESM builds. - Removes
tsconfig.build.json(previously used by the prior build tooling).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tsconfig.build.json | Removes the separate build tsconfig previously used to exclude specs/bench from compilation. |
| package.json | Adds conditional exports for ESM/CJS, updates entrypoint/type fields, and switches the build script to tsdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
While I appreciate the PR, and in fact would prefer it to be ESM only, it can't be changed in the current major version due to it being a breaking change. Anyone importing it will suddenly be switched to different source code and while it's unlikely it'll break due to how much backward compatibility stuff all the bundlers do, it's possible. Let me know if you think I'm incorrect on this though! Additionally this PR breaks some existing checks by removing the Finally, we could do a major version just to re-introduce ESM, but I would prefer to avoid it for now until I can resolve the ESM discussion with the express team, that way it's only one major in the case we can adopt ESM only. |
|
Adding ESM support seems fine to me for the next major version, once we only support Node.js versions that require(esm). And I’d say for Express 6 we should do the same—many are already moving in that direction, so it would make sense. |
My hope is that you're incorrect. This PR just introduces ESM which basically is "opt-in":
Which is exactly the point of this PR. But there are really two cases here: either whatever processes this code supports ESM or not. If it does, it's pretty straightforward and there are no traps awaiting. path-to-regexp is, architecture wise, trivial: a couple of exports bunched in a single module. I don't think there's a lot to break here.
I think to address this I'd need to know more about your concerns. I for one can't think of a case that could break here, and even more so, in a way that's unfixable while maintaining CJS+ESM builds.
This is where I don't have an answer for you yet - looks like ts-scripts doesn't give a lot of options on how to customize stuff. While you seem to be able to define multiple "projects", this does not solve CJS+ESM on its own. Additional scripts for renaming stuff would be needed here and that seems like not very elegant solution. I checked that tests defined in
That really depends on Node.js support. Do you support Node.js versions old enough not to support ESM at all? If yes, dual is the only option for now. If not, switching to ESM fully is frankly an implementation detail. Nonetheless, I figured going dual is the safest, fastest way to ensure that consumers would be able to enjoy better tree shaking and restored browser support without causing too much commotion. I firmy believe we can achieve this without breaking changes. |
If you do
And that's the breaking change. I understand the point of the PR and agree with it. I'm the one who introduced ESM here to begin with, and then removed it due to external requests, and am on board with adding it back in a new major, as I said, once conditions outside my control are resolved.
If you happen to use
Just use the git history, that's what it's there for: 9085eda |
|
Here's the breaking change. If you use Note the duplication into |
Apologies, I should have worded it better. It's "opt-in" from bundler/runtime perspective. It'd say: "I understand ESM, I see this package exposes ESM modules, so I'm gonna use it!".
I don't think I'm on the same page here. To me, a promise we give with semver is: "For whatever documented exports, we promise it will be there and it will continue to work exactly as documented". So as long as you don't need to do any code changes on consumer side for bumped version to work, it's not a breaking change. Am I in the wrong here?
👀
Ah! That's actually very valid case. I do it all the time with built-in modules. Thankfully, easily fixable. Done!
Duplication into default was already there, but only in CJS - as per above, fixed that. __esModule tag is back. I'd love to cover more edge cases. So far my test files are doing fine:
const p = require("./dist/index.js");
console.log({
p__esModule: p.__esModule,
pCompile: p.compile,
pDefault: p.default,
pDefaultCompile: p.default.compile,
});
import p, { compile } from "./dist/index.mjs";
console.log({
p,
pCompile: p.compile,
namedCompile: compile,
}); |
Closes #346
Fixes #347
This PR reintroduces an ESM build while preserving CJS compatibility for existing consumers.
The removal of the ESM build in v7 caused tree-shaking issues and may have affected adoption.
It is done so with utmost care for CJS compatibility:
typeremains to be impliedcommonjs,dist/index.js,dist/index.d.ts,index.js.mapare all there and remain to be CJS files,ESM is introduced via
moduleandexports. I leveraged tsdown, a zero-config bundler, that made this task easy and (hopefully) maintainable in the future.Unlike #397, this does NOT make this package "ESM-first" which, I assume, was the main concern ultimately leading to the PR being closed.
@arethetypeswrong/clireports no issues: