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

Feature request: require node "^12 || >=14" and support "type": "module" #299

Closed
TomerAberbach opened this issue May 16, 2021 · 4 comments
Closed

Comments

@TomerAberbach
Copy link

From what I can tell, it's not currently possible to run generators written as ESM modules/"type": "module". This is a feature request for that to be possible!

I took a look at the code and I think these lines in lib/store.js could be modified like so to support both CJS and ESM:

Object.defineProperty(this._generators, namespace, {
- get() {
-   const Generator = require(path);
+ async get() {
+   const Generator = await import(require.resolve(path));
    return Generator;
  },
  enumerable: true,
  configurable: true
});

Of course, that's not the end of the story. Then every usage of this.generators[something], directly or transitively, must be updated to support asynchronous importing of generators. I would expect some public API methods would be affected. Also, dynamic import's module resolution strategy is different than require's.

So basically, this would definitely be a breaking change, but I think it's an essential change now that ESM is becoming the preferred module format.

Let me know if you'd like any help with this. I'd be keen to help! 😃

@mshima
Copy link
Member

mshima commented May 16, 2021

Implemented today.

  • Why not keep ">=12.10.0"? Not planning to change it.

It won't be a breaking change.
Composing with a esm generator will have a different behavior, it will return a promise instead a generator.

@TomerAberbach
Copy link
Author

Implemented today.

Do you mean that ESM generators are already supported? I personally couldn't get it to work, but maybe I was doing something wrong.

  • Why not keep ">=12.10.0"? Not planning to change it.

I think that the earliest Node version where ESM, including dynamic import, is supported unflagged is v12.17.0. So you'd have to require ">= 12.17.0" at least if dynamic import is in your code (I think it's a syntax error prior to v12.17.0).

It won't be a breaking change.
Composing with a esm generator will have a different behavior, it will return a promise instead a generator.

Sure, I suppose you could try to have different behavior between CJS and ESM generators.

@mshima
Copy link
Member

mshima commented May 23, 2021

ESM support is added to 3.4.0

@TomerAberbach
Copy link
Author

Just tried it out! Confirmed it works 🥳

Great work! 😄

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

No branches or pull requests

2 participants