-
Notifications
You must be signed in to change notification settings - Fork 23
ADR: CommonJS and ESM decision #323
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
base: master
Are you sure you want to change the base?
Changes from 6 commits
2667117
cca6311
d8c9118
cda116a
b8d5a59
7fa2531
0f72b27
8007ef6
ca5dcb0
43b0ed8
166e5f8
9fe1075
4ca69bc
01d4903
68eb3d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
# ADR: CommonJS only output | ||
|
||
## Status | ||
|
||
Proposed | ||
|
||
## Submitters | ||
|
||
- @kjugi | ||
- @wesleytodd | ||
- @ctcpip | ||
|
||
## Decision Owners | ||
|
||
- @expressjs/express-tc | ||
|
||
## Context | ||
|
||
The document's objective is to gather all notable comments and thoughts in one place and track potential changes in this topic. We have noticed that it is repeated frequently in many issues from the community and we need to take action. | ||
|
||
Based on the comments in the issues we know that some of you might not feel happy about it. We have acknowledged the need and discussion around it touched on multiple scenarios. Including: | ||
- rethinking the process and exposing both paths for all libraries | ||
- expose both paths for selected libraries | ||
- keeping default settings as the main target is on the server | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
**Why do we need this decision?** | ||
- To reduce noise and maintain the ultimate answer for all of the issues. | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
**What problem does it solve or avoid?** | ||
- General response to community request. | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
**Are there any existing issues/discussions/pull requests related to this?** | ||
- https://github.com/pillarjs/router/issues/128 | ||
- https://github.com/expressjs/discussions/issues/297 | ||
- https://github.com/expressjs/express/discussions/6051 | ||
- https://github.com/jshttp/cookie/issues/211 | ||
|
||
## Decision | ||
|
||
During [working session](https://github.com/expressjs/discussions/issues/320) we have decided to not revisit, investigate or discuss this topic further. That means ESM exports won't be available for expressjs as well for pillarjs and jshttp packages. | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
**What will be done?** | ||
|
||
Future issues can be closed with a link to this document. | ||
|
||
## Rationale | ||
|
||
CommonJS is the default node.js syntax. The JS world moved in the ESM direction as browsers consumed it well, and bundlers could make a tree-shake feature and dynamic imports. There is still a lot of baggage within the ESM itself and the way how we can use it. | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
To keep it short it's a whole new chapter to discuss and consider so it could use a lot of time and resources to make it properly. Don't get this wrong - it's not impossible tho! Most of our users will use the package in their project and pass it to the bundler which will produce the right format without any issues. | ||
kjugi marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
||
- **Alternatives Considered:** | ||
- Alternative 1: Add ESM export to our libraries. CommonJS format is accepted by all most popular bundlers. | ||
- **Pros and Cons**: Outline the pros and cons of the chosen solution. | ||
- **Why is this decision the best option?** Time and energy can be shifted to other topics. | ||
|
||
## Consequences | ||
|
||
- **Positive Impact**: It does not require to support another set of tools and one more major (or at least big) release. | ||
- **Negative Impact**: | ||
- Packages can't be used in deno projects and potentially in other future runtime engines for JavaScript that decide to not support commonjs. That can be a potential user miss | ||
|
||
- OSS library authors that use our packages in ESM native libs might suffer from a lack of support | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a greater negative impact which is that other users fork the packages to ESM and those packages lack security updates over time which leaves users in a worse position. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a valid point! Added, thanks! Please resolve if it's fine now ✌️ |
||
- **Mitigations**: Potential decision update to support isomorphism for selected libraries (not specified yet) and exposing both types (CJS and ESM) | ||
|
||
## References | ||
|
||
Support for commonjs imports in ESM code is available in the node. Described in docs: | ||
- https://nodejs.org/api/esm.html#interoperability-with-commonjs | ||
|
||
Support for ESM modules imports in commonjs is available since node v20 behind the experimental flag and node v23 without a flag. Docs: | ||
- https://nodejs.org/api/modules.html#loading-ecmascript-modules-using-require | ||
|
||
## Changelog | ||
|
||
Track changes or updates to this ADR over time. Include the date, author, and a brief description of each change. | ||
|
||
- **[2025-01-15]**: [@kjugi] - document init |
Uh oh!
There was an error while loading. Please reload this page.