-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
feat: provide explicit ESM pkg entry points #560
base: main
Are you sure you want to change the base?
Conversation
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.
As proposed, this change would be ineffective as Node.js prefers "node"
over "import"
, and all other environments prefer "default"
over "node"
.
The "default"
browser endpoint is transpiled and tested to work with older browser environments (Chrome 87, Firefox 78, Safari 13.1), while the "node"
one targets Node.js environments specifically. Node.js also allows for importing CJS modules with named exports, so the DX of using only CJS for Node.js is the same as using ESM would be, with the benefit of avoiding dual package hazards.
It's been a while since this part of the package config saw any changes, so I'd be willing to review it again. Is there any situation where the current config can be shown to be suboptimal in some way? To be explicit, I'm interested in practical rather than theoretical cases.
My fault, I was a bit hasty, trying to solve a couple of problems at once. Our practical cases:
|
390cad6
to
b473856
Compare
Could you provide some links explaining how ESM is better than CJS for this? Searching for "ISEC policies" isn't providing anything useful for me, and I'm rather unconvinced about the differences being exploitable as an actually viable attack vector.
What's the downside in this case? Presumably you're compiling with |
EMS format brings a portion immutability: you cannot just override Our case is quite exotic: we want to resolve a deps tree as esm, but to convert it to cjs at the final step. This will significantly reduce the size of the bundle. |
The increase in security is wholly illusory. Consider the following, for instance: // patch.mjs
import { Document } from 'yaml'
Document.prototype.toJS = () => 'foo' // index.mjs
import './patch.mjs'
import { parse } from 'yaml'
parse('one: two') // returns 'foo' If an attacker is able to execute code in your runtime environment, the above will work completely independently of the CJS/ESM packaging of this library. This is not a scenario for which it's reasonable to build protections; if an attacker gets that far, you've already lost the game. To protect against this, you should have wholly separate environments where you're running user & system code.
Where is that bundle size saving coming from, for something like |
Well, immutable references in the module cache are just the first step, but more are needed. warmup + deep Object.freeze might fit your snippet.
Correct. We have a need to make static builds for some utilities. |
Given that I'm not willing to deep-freeze all objects in This security policy sounds like a thing that you can easily measure, but which has no real-world impact.
Adding separate import & require entry points for Node.js would probably need to be done in a new major version, given that it would break the following identity: // barrel.cjs
module.exports = require('yaml') // index.mjs
import { Document as ESMDocument } from 'yaml'
import { Document as CJSDocument } from './barrel.cjs'
ESMDocument === CJSDocument So while adding an entrypoint sounds like a small change, it's still a breaking change. Which, to be clear, I'm quite willing to consider, but it needs to have really good reasons for it. That's why I'm asking for practical, real-world examples of cases where the current config can be shown to be suboptimal in some way. |
Reasonable. But module refs obtained via var y1 = require('yaml'), y2 = await import('yaml'); console.log(y1 === y2) We can avoid any hypothetical br change by adding smth like: {
"export": {
"esm": {
"types": "./dist/index.d.ts",
"default": "./dist/browser/index.js"
},
// ... rest
}
} |
The top-level module, yes, but not the contents. Try this: var y1 = require('yaml');
import('yaml').then(y2 => {
console.log(y1.parse === y2.parse);
}); |
Same trick with a separated esm entrypoint: |
Ah, I'd missed that you'd changed your proposal to defining I don't buy the argument that ESM is more secure than CJS for this library, and I'm still waiting for a reply to this question from earlier:
Tbh, I'm tempted to wait for Node's require(esm) support to mature enough to allow dropping the CJS build completely. |
I don't claim to have the truth. But we find the combination sufficient: immutable module cache plus frozen immutable modules. I want to believe that |
512d83d
to
04e5154
Compare
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.
Please review my last comment.
Despite that the cjs format is widely used in Nodejs, there are many reasons to use ESM today. Let's add the corresponding entry points.