-
-
Notifications
You must be signed in to change notification settings - Fork 378
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
fix: Fix require(ESM) for Node v22 #722
Conversation
Fixes for Node 22: sindresorhus#696 sindresorhus#671 sindresorhus#668 sindresorhus#661 sindresorhus#600 ... and others
This is not correct. |
So what is the correct solution to make it work in Node 22? I tried to explain my reasons here: #696 (comment) I will be happy to fix it, but I do not know what you consider to be correct. |
@sindresorhus May I ask what the correct solution is? |
Maybe to use |
I'll try to answer your question. file-type is a pure ECMAScript Module (ESM). There is nothing wrong with the
loading ESM module with Node
|
We have already discussed it #723 |
I did some more investigation, and I think there is something we can do to make this work. Node.js introduced a new condition introduced "module-sync", which seems to tackle this issue. "exports": {
".": {
"node": {
"types": "./index.d.ts",
"import": "./index.js",
"module-sync": "./index.js"
},
"default": {
"types": "./core.d.ts",
"import": "./core.js",
"module-sync": "./core.js"
}
},
"./core": {
"types": "./core.d.ts",
"import": "./core.js",
"module-sync": "./core.js"
}
} @sindresorhus, any objection we reopen this PR, and use |
I don't want to guarantee that. |
As far as I can see we do not have any top-level await in the module graph. Therefor I see no issue declaring that this is the case, and that file-type it totally fine to be loaded as an ES Module via require. I wish they made this obvious option possible years ago. Note that most your ESM modules are exporting their entry points via 3/4 file-type dependencies (@tokenizer/inflate, strtok3, token-type) I maintain, and these are not using top-level await. Number 4 is one you own, uint8array-extras, which neither has any top level await, and is already implicit I like to have this one in very much, as I build my open source code components to be easy accessible and easy to use on as many platforms as possible. I really see no harm in this change, and if we experience issues, we always change course. |
None now, but could be in the future. I don't want to be held back by
All modern platforms support ESM. It seems like this is an issue with Nest.js and it's not something we should have to deal with: nestjs/nest#13319 (comment) |
There is always a risk that dependency breaks our code. And there are plenty of ways to deal with that.
It does harm not if we allow esm-require for the time being, while the world is still very slowly migrating to ESM. It it is totally up to use for how long and under which condition we support that. But for this moment I am committed to do so. |
Hi all, I believe this is good change by Node, and it will help so many older projects who can't migrate to full ESM easily. so please consider adding its support (as it seems top level await is not being used). Thanks. |
Fixes for Node 22: