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

feat!: move from CommonJS to ESM #3705

Closed
escapedcat opened this issue Oct 20, 2023 · 10 comments · Fixed by #3850
Closed

feat!: move from CommonJS to ESM #3705

escapedcat opened this issue Oct 20, 2023 · 10 comments · Fixed by #3850

Comments

@escapedcat
Copy link
Member

escapedcat commented Oct 20, 2023

Expected Behavior

Should update all the code, like it was done here: https://github.com/conventional-changelog/conventional-changelog/pull/1144/files

@joberstein
Copy link
Contributor

@escapedcat is this still something that you'd like assistance with, and if so, would you prefer everything be converted to ESM all in one PR, or is conversion in piecemeal per module okay for more complicated conversions (e.g. a PR just for @commitlint/resolve-extends)?

@escapedcat
Copy link
Member Author

@escapedcat is this still something that you'd like assistance with, and if so, would you prefer everything be converted to ESM all in one PR, or is conversion in piecemeal per module okay for more complicated conversions (e.g. a PR just for @commitlint/resolve-extends)?

Yes, happy for any help.

I'm not sure if commitlint can be run with some packages in CommonJS and some in ESM. If so, this might be preferred, right?
If not we could create a feature branch and create single PRs to merge each module into that one till it's finished.
Risk is always that fixes might be added to modules which are already converted. But tbh it's not that many coming in and once the process started we could freeze the CommonJS codebase till the change happened.

wdyt?

@joberstein
Copy link
Contributor

joberstein commented Nov 18, 2023

@escapedcat is this still something that you'd like assistance with, and if so, would you prefer everything be converted to ESM all in one PR, or is conversion in piecemeal per module okay for more complicated conversions (e.g. a PR just for @commitlint/resolve-extends)?

Yes, happy for any help.

I'm not sure if commitlint can be run with some packages in CommonJS and some in ESM. If so, this might be preferred, right? If not we could create a feature branch and create single PRs to merge each module into that one till it's finished. Risk is always that fixes might be added to modules which are already converted. But tbh it's not that many coming in and once the process started we could freeze the CommonJS codebase till the change happened.

wdyt?

Yeah I believe it's considered a breaking change from moving to CJS -> ESM because a project can't require ESM modules and any commitlint project could technically be published on its own. So if you wanted to do incremental changes, you'd want to bump the major version for the affected packages at minimum, and I think the rest of the packages using it could still be on CJS as long as they change their require references to import, which is not a breaking change for consumers. But that depends on how easy it is to bump individual packages differently.

@commitlint/load@18.4.2 => 19.0.0 (converted to ESM)
// Breaking changes for direct callers so major bump

@commintlint/cli@18.4.2 => 18.5.0 (still on CJS)
// It's already being loaded with import, 
// but you can imagine if this was `const {load} = require('@acommitlint/load')`
// that it would be a non-breaking change to change the reference 
// and bump the @commitlint/load version.
import load from '@commitlint/load';

If the versioning seems like it'll be tricky to adjust, the separate long-standing feature branch makes sense, just might have the pain of merge conflicts when it's ready to be merged in. But I think in either approach, making a PR for each module conversion to ESM separately would be easier to digest since it will probably involve a lot of files.

@sheerlox
Copy link

sheerlox commented Nov 18, 2023

I am quickly mentioning import-from-esm which might be useful during the conversion to ESM.

I developed that package while working on ESM presets support for semantic-release because import-from doesn't support ESM (and I noticed it being used in @commitlint/config-lerna-scopes). It also detects when it's about to import a JSON module and falls back to require to avoid using the still-experimental import assertions.

Please note that it only supports default exports for now (which for CJS is the whole module), but if supporting ESM named exports is needed, please open an issue on the repo and we can work on designing that feature!

Also, if ESM presets are needed to check they're working with commitlint (which for shared configs doesn't seem to be the case for now), you can take a look at insurgent-lab/conventional-changelog-preset and insurgent-lab/commitlint-config (ESM branch) (mentioning this since I don't think there's a lot of ESM presets out there, maybe that's even the only one 😅)

@joberstein
Copy link
Contributor

I am quickly mentioning import-from-esm which might be useful during the conversion to ESM.

I developed that package while working on ESM presets support for semantic-release because import-from doesn't support ESM (and I noticed it being used in @commitlint/config-lerna-scopes). It also detects when it's about to import a JSON module and falls back to require to avoid using the still-experimental import assertions.

Please note that it only supports default exports for now (which for CJS is the whole module), but if supporting ESM named exports is needed, please open an issue on the repo and we can work on designing that feature!

Also, if ESM presets are needed to check they're working with commitlint (which for shared configs doesn't seem to be the case for now), you can take a look at insurgent-lab/conventional-changelog-preset and insurgent-lab/commitlint-config (ESM branch) (mentioning this since I don't think there's a lot of ESM presets out there, maybe that's even the only one 😅)

@sheerlox I'm not too well-versed in how this package is being used, but is there still a gap that exists that import-from address which can't be filled with node:path and await import(...)? As I understand it, import-from uses a given directory to call the import from, but I'm unclear on how that's different from passing path.resolve/relative or something similar to await import. Could you provide some additional context in case I'm missing something?

@sheerlox
Copy link

sheerlox commented Nov 19, 2023

The main benefit of using import-from is that it abstracts the need to resolve the path and create a require statement. Its code is really straightforward:

(fromDirectory, moduleId) => createRequire(path.resolve(fromDirectory, 'noop.js'))(moduleId);

In the case of import-from-esm, there are a few additional benefits because of the way ESM works:

  1. importing a package installed along a library (in the parent application) from that library is no longer possible (which was the issue that made me work on this library). You need to use import.meta.resolve, which is behind an experimental flag (although there's a ponyfill available at wooorm/import-meta-resolve, which import-from-esm uses under-the-hood).
  2. if the file you're trying to import (whether relative, package, export map, etc ...) is a JSON file, you need to detect that and use import assertions or require (while the former is still in experimental).
  3. file extensions are now mandatory for relative paths. import-from-esm re-introduces require's file extension discovery.

As you can see, there is quite a bit of complexity that is abstracted behind import-from-esm. The first bullet point issue affected both @semantic-release/commit-analyzer and @semantic-release/release-notes-generator. After spending hours on research to solve the issue, I realized that the work I was doing would benefit others as well, so I decided to create a package out of it.

As a proponent of ESM, I have put a lot of thought into poly-filling require features for import, but finally came to the conclusion that developing a package to facilitate the ecosystem transition to ESM by reducing friction was a good thing. I have already planned to deprecate the optional file extension feature in a major release once ESM adoption becomes more widespread.

@joberstein
Copy link
Contributor

The main benefit of using import-from is that it abstracts the need to resolve the path and create a require statement. Its code is really straightforward:

(fromDirectory, moduleId) => createRequire(path.resolve(fromDirectory, 'noop.js'))(moduleId);

In the case of import-from-esm, there are a few additional benefits because of the way ESM works:

  1. importing a package installed along a library (in the parent application) from that library is no longer possible (which was the issue that made me work on this library). You need to use import.meta.resolve, which is behind an experimental flag (although there's a ponyfill available at wooorm/import-meta-resolve, which import-from-esm uses under-the-hood).
  2. if the file you're trying to import (whether relative, package, export map, etc ...) is a JSON file, you need to detect that and use import assertions or require (while the former is still in experimental).
  3. file extensions are now mandatory for relative paths. import-from-esm re-introduces require's file extension discovery.

As you can see, there is quite a bit of complexity that is abstracted behind import-from-esm. The first bullet point issue affected both @semantic-release/commit-analyzer and @semantic-release/release-notes-generator. After spending hours on research to solve the issue, I realized that the work I was doing would benefit others as well, so I decided to create a package out of it.

As a proponent of ESM, I have put a lot of thought into poly-filling require features for import, but finally came to the conclusion that developing a package to facilitate the ecosystem transition to ESM by reducing friction was a good thing. I have already planned to deprecate the optional file extension feature in a major release once ESM adoption becomes more widespread.

Thanks for the explanation! It sounds like the behavior difference is mostly in the extension handling rather than resolving file paths. Is it correct to say that require needs extra support to handle resolving other packages installed in a parent package, but import can handle that on its own with import.meta.resolve?

@sheerlox
Copy link

sheerlox commented Nov 30, 2023

Is it correct to say that require needs extra support to handle resolving other packages installed in a parent package, but import can handle that on its own with import.meta.resolve?

Both need extra support: require needs to be created through createRequire, and import needs to be passed a full module path resolved with import.meta.resolve beforehand.

Here's the relevant (and simplified) part of the code from import-from-esm, with fromDirectory and moduleId as input variables:

import { resolve } from 'node:path';
import { pathToFileURL } from 'node:url';
import { moduleResolve } from 'import-meta-resolve';

const parentModulePath = pathToFileURL(resolve(fromDirectory, 'noop.js'));

try {
    const resolvedModule = moduleResolve(moduleId, parentModulePath, new Set(['node', 'import']));
    const loadedModule = await import(resolvedModule);
  } catch (error) {
    debug(`Failed to resolve '${moduleId}' from '${baseURL.href}': ${String(error)}`);
  }

Support for loading JSON modules also needs explicit handling in ESM (preferably by falling back to require while the import attributes are experimental).

@jjangga0214
Copy link

Just a short note: cosmiconfig supports loading esm as well.

@escapedcat
Copy link
Member Author

Linking #3850 here

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

Successfully merging a pull request may close this issue.

4 participants