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

Esm plugins #49

Merged
merged 4 commits into from
Apr 1, 2021
Merged

Esm plugins #49

merged 4 commits into from
Apr 1, 2021

Conversation

wooorm
Copy link
Member

@wooorm wooorm commented Mar 27, 2021

Add support for ESM plugins/presets

ESM modules can use export default to expose their plugin or preset.

This does not add support for config files in ESM just yet.

  • Add support for plugins in ESM format w/ an .mjs extension
  • Add support for plugins in ESM format w/ a .js extension if the nearest
    package.json has a type: 'module'
  • Add support for interop bundles (CJS w/ __esModule: true field)

/cc @ChristianMurphy @remcohaszing

Based somewhat on Babel: https://github.com/babel/babel/blob/d04842a70031fe91656ba3454e7b6a04f4fedc42/packages/babel-core/src/config/files/module-types.js

ESM modules can use `export default` to expose their plugin or preset.

This does not add support for config files in ESM just yet.

* Add support for plugins in ESM format w/ an `.mjs` extension
* Add support for plugins in ESM format w/ a `.js` extension if the nearest
  `package.json` has a `type: 'module'`
* Add support for interop bundles (CJS w/ `__esModule: true` field)
@wooorm wooorm added 🦋 type/enhancement This is great to have 🧒 semver/minor This is backwards-compatible change 🗄 area/interface This affects the public interface 🙆 yes/confirmed This is confirmed and ready to be worked on labels Mar 27, 2021
lib/configuration.js Show resolved Hide resolved
lib/configuration.js Show resolved Hide resolved
if (result && typeof result === 'object' && result.__esModule) {
result = result.default
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CJS can be import()ed as well. module.exports is exported as the default export. I think it’s best to just let NodeJS decide how to parse the imported file.

The biggest difference is import() can’t be used for JSON files, which I think is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. True. I think dynamic import is a later Node addition though? I’m guessing that, or some other good reason, is why Babel is doing this method.

The biggest difference is import() can’t be used for JSON files, which I think is fine.

Ah, that‘s also currently possible (though not ideal/recommended). So that’s also breaking.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dynamic import is a later Node addition though?

Both were dynamic and static imports were declared stable in Node 13 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#import

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m very open to making this better, and indeed letting Node decide, but I don’t want to break things with this feature. I’m afraid it will.

P.S. Also this is scary to me: https://github.com/babel/babel/blob/d04842a70031fe91656ba3454e7b6a04f4fedc42/packages/babel-core/src/config/files/module-types.js#L10-L13. But our tests are running on dubium, and that seems fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dynamic imports were added in NodeJS 12.x.x and 13.x.x, at around the same time, because 12 was LTS and 13 was latest at the time.

$ docker run -ti --rm node:12.0.0-alpine        
Welcome to Node.js v12.0.0.
Type ".help" for more information.
> import('fs')
Promise {
  <rejected> Error: Not supported
      at repl:1:1
      at Script.runInThisContext (vm.js:123:20)
      at REPLServer.defaultEval (repl.js:358:29)
      at bound (domain.js:415:14)
      at REPLServer.runBound [as eval] (domain.js:428:12)
      at REPLServer.onLine (repl.js:665:10)
      at REPLServer.emit (events.js:201:15)
      at REPLServer.EventEmitter.emit (domain.js:471:20)
      at REPLServer.Interface._onLine (readline.js:314:10)
      at REPLServer.Interface._line (readline.js:691:8)
}
> (node:1) UnhandledPromiseRejectionWarning: Error: Not supported
    at repl:1:1
    at Script.runInThisContext (vm.js:123:20)
    at REPLServer.defaultEval (repl.js:358:29)
    at bound (domain.js:415:14)
    at REPLServer.runBound [as eval] (domain.js:428:12)
    at REPLServer.onLine (repl.js:665:10)
    at REPLServer.emit (events.js:201:15)
    at REPLServer.EventEmitter.emit (domain.js:471:20)
    at REPLServer.Interface._onLine (readline.js:314:10)
    at REPLServer.Interface._line (readline.js:691:8)
(node:1) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). (rejection id: 1)
(node:1) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
>
$ docker run -ti --rm node:12-alpine
Welcome to Node.js v12.21.0.
Type ".help" for more information.
> import('fs')
Promise { <pending> }
> 

This is the same thing as xdm not working in node 14.0.0. Your conclusion then was that people should just upgrade their Node version to the latest non-major version. This is the same thing.

This also affects how other file types are loaded. I.e. how should .ts files be handled? I’d say however NodeJS is configured using loaders / register. Currently this enforces these files to be loaded using require. I realize this is an edge case, but using this to load JSON files seems to be even more of an edge case.


Side note: Doesn’t this logic belong in load-plugin?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same thing as xdm not working in node 14.0.0. Your conclusion then was that people should just upgrade their Node version to the latest non-major version. This is the same thing.

I think they’re different. xdm is specifically ESM. This is a CJS project. I first want to at least support plugins in ESM format, before porting the whole project itself over to ESM.
Here’s an example: https://github.com/wooorm/iso-3166#matrix. The details and table are generated by a little plugin: https://github.com/wooorm/iso-3166/blob/main/build-iso-3166-1-a2-table.js. unified-engine should support plugins as ESM first, IMO, in a minor release. Porting the engine and remark/rehype/other stuff to ESM later, is another problem.

This also affects how other file types are loaded. I.e. how should .ts files be handled? I’d say however NodeJS is configured using loaders / register. Currently this enforces these files to be loaded using require. I realize this is an edge case, but using this to load JSON files seems to be even more of an edge case.

This could be made configurable in the future. But because Node’s require has supported JSON by default, I don’t see a reason to remove that support. Node hasn’t supported loading .ts files. (Although it could be injecting something to handle them in `require.extensions['.ts']).

Side note: Doesn’t this logic belong in load-plugin?

Maybe. Probably too. That would honor export maps I guess, and is more involved.
This is a more naive attempt, that would cover most cases. If Node says a required file is ESM, try using ESM.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other projects are using the same method btw: jestjs/jest#11167. I think it’s wise to stick with it

lib/configuration.js Show resolved Hide resolved
lib/configuration.js Show resolved Hide resolved
if (result && typeof result === 'object' && result.__esModule) {
result = result.default
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think dynamic import is a later Node addition though?

Both were dynamic and static imports were declared stable in Node 13 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules#import

test/configuration-plugins.js Outdated Show resolved Hide resolved
@codecov-io

This comment has been minimized.

@wooorm wooorm merged commit 1091e32 into main Apr 1, 2021
@wooorm wooorm deleted the esm-plugins branch April 1, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗄 area/interface This affects the public interface 🧒 semver/minor This is backwards-compatible change 🦋 type/enhancement This is great to have 🙆 yes/confirmed This is confirmed and ready to be worked on
Development

Successfully merging this pull request may close these issues.

4 participants