-
-
Notifications
You must be signed in to change notification settings - Fork 757
Improve ESM compatibility #697
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?
Conversation
Static exports allow Node.js cjs-module-lexer to detect named exports correctly, improving ESM interop and bundler behavior.
Add an exports field to package.json exposing json, raw, text,
and urlencoded parsers as subpath exports.
This allows consumers to import individual parsers directly:
- CommonJS: require('body-parser/json')
- ESM: import json from 'body-parser/json'
The change is fully backward compatible and enables users to
opt into loading only the parsers they need.
| }, | ||
| "exports": { | ||
| ".": "./index.js", | ||
| "./package.json": "./package.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be necessary to add the package.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added ./package.json to the exports field to avoid breaking existing tooling and user setups that rely on require('body-parser/package.json') to read metadata (e.g. version info). Once exports is defined, Node.js treats it as an allowlist, so this preserves backward compatibility.
Docs: https://nodejs.org/api/packages.html#package-entry-points
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im concerned here that the exports inclusion makes this breaking to a small number of users. I am always wary of packaging changes mid major (ala files, exports), as it can break things unless the paths cover everything that was possible previously.
There are indeed apps in the wild doing silly things like importing from lib which this change would break:
https://github.com/search?q=%22require%28%27body-parser%2F%22&type=code
(I think VSCode's auto-complete for imports often does these deep imports when there's no exports field, i've seen it a lot at a previous job. likely how folks ended up w/ this)
We can split out the index.js change as it's own PR, as that's standalone and won't break anyone. Or we can do an exhaustive exports field inside this same major. But that doesn't seem valuable here.
From the Node docs, we could do this to preserve back compat:
"exports": {
".": "./index.js",
"./package.json": "./package.json",
"./json": "./lib/types/json.js",
"./raw": "./lib/types/raw.js",
"./text": "./lib/types/text.js",
"./urlencoded": "./lib/types/urlencoded.js",
// Back compat entries start here
"./lib/*": "./lib/*.js", // will include read.js and utils.js, both internal but both still requireable today
"./lib/*.js": "./lib/*.js",
"./lib/types/*": "./lib/types/*.js",
"./lib/types/*.js": "./lib/types/*.js"
}
Ref: #666 (comment)
This PR contains 2 commits:
fix: use static exports instead of lazy getters
Static exports allow Node.js
cjs-module-lexerto detect namedexports correctly, improving ESM interop and bundler behavior.
feat: add subpath exports for individual parsers
Add an exports field to package.json exposing json, raw, text,
and urlencoded parsers as subpath exports.
This allows consumers to import individual parsers directly:
The change is fully backward compatible and enables users to
opt into loading only the parsers they need.
Closes #666