Skip to content
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 #723

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

@Borewit
Copy link
Collaborator

Borewit commented Jan 16, 2025

Your PR would mix ESM code with CommonJS.

As stated in the dcoumentation file-type is a pure ESM module

We recommend to migrate your project to ESM as well .

If you do want to stick with CommonJS for whatever reason, this is the way to import file-type:

const { fileTypeFromFile } = await import('file-type');

Which may not work in TypeScript, see load-esm how to resolve that.

@mdorda
Copy link
Author

mdorda commented Jan 17, 2025

@Borewit Thank you for your reply. I do not think it mixes ESM with CJS. It just adjust loading this package to new Node.js feature. This package remains pure ESM (which is good decisions, by the way).

To use load-esm is not an option for many reasons. Such as static dependency analysis, avoiding dynamic imports, code readability, etc. This PR is a correct and official solution for a pure ESM packages for Node 22 and newer.

@Borewit
Copy link
Collaborator

Borewit commented Jan 17, 2025

I do not think it mixes ESM with CJS. It just adjust loading this package to new Node.js feature. This package remains pure ESM (which is good decisions, by the way).

What I don't understand, you link to our JavaScript ESM files in the require entry point. Hence my remark it is mixed. If that is valid, why is require not simply using the import entry point?

This PR is a correct and official solution for a pure ESM packages for Node 22 and newer.

I am eager to learn, can you provide a link to that official guidance?

@mdorda
Copy link
Author

mdorda commented Jan 17, 2025

What I don't understand, you link to our JavaScript ESM files in the require entry point. Hence my remark it is mixed. If that is valid, why does require not simply using the import entry point?

No, I use it in TS and Nest.js. So my code looks like:

import { fileTypeFromFile } from 'file-type';

...
    private async validateImage(filePath: string): Promise<void> {
        const fileType = await fileTypeFromFile(filePath);
...

Unfortunately, Nest.js supports only CJS and does not support ESM. Happilly, Node.js solve this issue. But this package does not support it :-(

I think this code is much more clearer than what you suggest:

import loadEsm from 'load-esm';

...
    private async validateImage(filePath: string): Promise<void> {
        const { fileTypeFromFile } = loadEsm('file-type');
        const fileType = await fileTypeFromFile(filePath);
...

I am eager to learn, can you provide a link to that official guidance?

See https://socket.dev/blog/node-js-delivers-first-lts-with-require-esm-enabled
NPM package needs to have main, exports.[].default or exports.[].require. That is all. Nothing more is needed.

We already use many pure ESM packages without anu problem. Such as geojson-vt, kdbush or supercluster.

I just thought that the point of opensource is to contribute to the development of a project if it helps a lot of people. Which in the case of Nest.js it really is. It doesn't break anything for you, and judging by the amount of ignored issues, I'm not the only one who appreciates this. This PR simply adds support for a new Node.js feature. That's all. I'm not picking a fight over ESM or CJS here. We've got everything we can in ESM. I understand the benefits, but Nest.js obviously doesn't.

Why to use hacky load-ems when there is a clean and official solution?

@Borewit
Copy link
Collaborator

Borewit commented Jan 17, 2025

We differentiate in the file-type "exports" between Node.js and default JavaScript engines, to preserver Node.js specific functions for the Node.js engine, and consistently we export the associated TypeScript typings.

The typings are exported via "types" and the ESM JavaScript code via "import".
"require" is to my understanding designed to export CommonJS based code, where dependencies are being imported via require().

The only thing I can derive from the documentation you provided is that, the still experimental feature, require(esm)
is enabled by default in Node.js 22. That require(esm) should be able to load ESM modules as well.

It is also clear it does not work for file-type (yet). I also believe that the reason for that is indeed the way we export. But that is not because we forgot or should wire require in the "exports". The "exports" "require" is to export CommonJS/require() type JavaScript , and not ESM/import() JavaScript. To me this appears to be a shortcoming in the require(esm), as file-type is a valid ESM module.

@mdorda
Copy link
Author

mdorda commented Jan 17, 2025

Thanks for the constructive reply. Fair enought. It's entirely up to you, we'll fork the package if we have to.

@Borewit
Copy link
Collaborator

Borewit commented Jan 17, 2025

As a side note @mdorda , I do appreciate you are reaching out, trying to make things better for everyone. I totally see the frustration for many users, who just simply want to be focus on using the modules rather then being stuck on interoperability issues between CommonJS / ESM. We are discussing here, not fighting. I value your input and contribution.

This is a different discussion, but load-esm can go in waste bin if TypeScript stops transpiling dynamic import to require. It's indeed simply provided as a workaround to unblock users, and to prevent spreading workarounds over each and every module in the NPM ecosystem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants