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.

Sign up for GitHub

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

Jump to bottom

Esm plugins #49

Merged
merged 4 commits into from Apr 1, 2021
Merged

Esm plugins #49

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
998f6f4
Add support for async config creation
wooorm Mar 27, 2021
21d215a
Add support for ESM plugins/presets
wooorm Mar 27, 2021
20e4c1e
Remove ESM tests on Node 10
wooorm Mar 27, 2021
cc4f76c
Fix esm check in tests
wooorm Apr 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
87 changes: 53 additions & 34 deletions lib/configuration.js
View file
Open in desktop
Expand Up @@ -74,11 +74,13 @@ function load(filePath, callback) {
return callback(error, file)
}

callback(null, self.create())
self.create().then(function (result) {
callback(null, result)
}, callback)
wooorm marked this conversation as resolved.
Show resolved Hide resolved
}
}

function create(buf, filePath) {
async function create(buf, filePath) {
var self = this
var fn = (filePath && loaders[path.extname(filePath)]) || defaultLoader
var options = {prefix: self.pluginPrefix, cwd: self.cwd}
wooorm marked this conversation as resolved.
Show resolved Hide resolved
Expand All @@ -100,21 +102,21 @@ function create(buf, filePath) {

if (contents === undefined) {
if (self.defaultConfig) {
merge(
await merge(
result,
self.defaultConfig,
Object.assign({}, options, {root: self.cwd})
)
}
} else {
merge(
await merge(
result,
contents,
Object.assign({}, options, {root: path.dirname(filePath)})
)
}

merge(result, self.given, Object.assign({}, options, {root: self.cwd}))
await merge(result, self.given, Object.assign({}, options, {root: self.cwd}))

return result
}
Expand Down Expand Up @@ -149,26 +151,22 @@ function loadJson(buf, filePath) {
return result
}

function merge(target, raw, options) {
async function merge(target, raw, options) {
if (typeof raw === 'object' && raw !== null) {
addPreset(raw)
await addPreset(raw)
} else {
throw new Error('Expected preset, not `' + raw + '`')
}

return target

function addPreset(result) {
async function addPreset(result) {
var plugins = result.plugins

if (plugins === null || plugins === undefined) {
// Empty.
} else if (typeof plugins === 'object' && plugins !== null) {
if ('length' in plugins) {
addEach(plugins)
} else {
addIn(plugins)
}
await ('length' in plugins ? addEach(plugins) : addIn(plugins))
} else {
throw new Error(
'Expected a list or object of plugins, not `' + plugins + '`'
Expand All @@ -178,59 +176,80 @@ function merge(target, raw, options) {
target.settings = Object.assign({}, target.settings, result.settings)
}

function addEach(result) {
async function addEach(result) {
var index = -1
var value

while (++index < result.length) {
value = result[index]

if (value !== null && typeof value === 'object' && 'length' in value) {
use.apply(null, value)
} else {
use(value)
}
// Keep order sequential instead of parallel.
// eslint-disable-next-line no-await-in-loop
await (value !== null && typeof value === 'object' && 'length' in value
? use.apply(null, value)
: use(value))
wooorm marked this conversation as resolved.
Show resolved Hide resolved
}
}

function addIn(result) {
async function addIn(result) {
var key

for (key in result) {
use(key, result[key])
// Keep order sequential instead of parallel.
// eslint-disable-next-line no-await-in-loop
await use(key, result[key])
wooorm marked this conversation as resolved.
Show resolved Hide resolved
}
}

function use(usable, value) {
async function use(usable, value) {
if (typeof usable === 'string') {
addModule(usable, value)
await addModule(usable, value)
} else if (typeof usable === 'function') {
addPlugin(usable, value)
} else {
merge(target, usable, options)
await merge(target, usable, options)
}
}

function addModule(id, value) {
async function addModule(id, value) {
var fp = loadPlugin.resolve(id, {cwd: options.root, prefix: options.prefix})
var ext
var result

if (fp) {
try {
result = require(fp)
} catch (error) {
throw fault(
'Cannot parse script `%s`\n%s',
path.relative(options.root, fp),
error.stack
)
ext = path.extname(fp)

/* istanbul ignore next - To do next major: Tests don’t run on Node 10 */
if (ext !== '.mjs') {
try {
result = require(fp)
} catch (error) {
if (ext !== '.cjs' && error.code === 'ERR_REQUIRE_ESM') {
ext = '.mjs'
} else {
throw fault(
'Cannot parse script `%s`\n%s',
path.relative(options.root, fp),
error.stack
)
}
}

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


/* istanbul ignore next - To do next major: Tests don’t run on Node 10 */
if (ext === '.mjs') {
result = (await import(fp)).default
}

try {
if (typeof result === 'function') {
addPlugin(result, value)
} else {
merge(
await merge(
target,
result,
Object.assign({}, options, {root: path.dirname(fp)})
Expand Down
73 changes: 41 additions & 32 deletions lib/find-up.js
View file
Open in desktop
Expand Up @@ -4,6 +4,7 @@ var fs = require('fs')
var path = require('path')
var fault = require('fault')
var debug = require('debug')('unified-engine:find-up')
var wrap = require('trough/wrap')

module.exports = FindUp

Expand Down Expand Up @@ -68,23 +69,32 @@ function load(filePath, callback) {
result.code = 'ENOENT'
result.path = error.path
result.syscall = error.syscall
loaded(result)
} else {
try {
result = self.create(buf, self.givenFilePath)
debug('Read given file `%s`', self.givenFilePath)
} catch (error_) {
result = fault(
'Cannot parse given file `%s`\n%s',
path.relative(self.cwd, self.givenFilePath),
error_.stack
wrap(self.create, onparse)(buf, self.givenFilePath)
}

function onparse(error, result) {
if (error) {
debug(error.message)
loaded(
fault(
'Cannot parse given file `%s`\n%s',
path.relative(self.cwd, self.givenFilePath),
error.stack
)
)
debug(error_.message)
} else {
debug('Read given file `%s`', self.givenFilePath)
loaded(result)
}
}

givenFile = result
self.givenFile = result
applyAll(cbs, result)
function loaded(result) {
givenFile = result
self.givenFile = result
applyAll(cbs, result)
}
}

function find(directory) {
Expand Down Expand Up @@ -117,41 +127,40 @@ function load(filePath, callback) {

function done(error, buf) {
var fp = path.join(directory, self.names[index])
var contents

/* istanbul ignore if - Hard to test. */
if (error) {
if (error.code === 'ENOENT') {
return next()
}

error = fault(
'Cannot read file `%s`\n%s',
path.relative(self.cwd, fp),
error.message
)
debug(error.message)
return found(error)
}

try {
contents = self.create(buf, fp)
} catch (error_) {
return found(
fault(
'Cannot parse file `%s`\n%s',
'Cannot read file `%s`\n%s',
path.relative(self.cwd, fp),
error_.message
error.message
)
)
}

/* istanbul ignore else - maybe used in the future. */
if (contents) {
debug('Read file `%s`', fp)
found(null, contents)
} else {
next()
wrap(self.create, onparse)(buf, fp)

function onparse(error, result) {
if (error) {
found(
fault(
'Cannot parse file `%s`\n%s',
path.relative(self.cwd, fp),
error.message
)
)
} else if (result) {
debug('Read file `%s`', fp)
found(null, result)
} else {
next()
}
}
}

Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@
"remark-cli": "^9.0.0",
"remark-preset-wooorm": "^8.0.0",
"remark-toc": "^7.0.0",
"semver": "^6.0.0",
"strip-ansi": "^6.0.0",
"tape": "^5.0.0",
"unified": "^9.0.0",
Expand Down