Add partial support for “modules” preset option #8
Conversation
dinoboff
force-pushed
the
feat/modules
branch
2 times, most recently
from
ca12ed0 to
48530d2
Compare
There was a problem hiding this comment.
Wow, this is great!
Could you document the option in the README?
https://github.com/avajs/babel-preset-stage-4/blob/699ae236caea1b529e8ec8f527bb385325d84d17/package-hash.js#L4:L5 should be updated to always include the babel-plugin-transform-es2015-modules-commonjs plugin.
index.js
Outdated
| @@ -1,7 +1,10 @@ | |||
| 'use strict'; | |||
| /* eslint-disable import/no-dynamic-require */ | |||
| module.exports = () => { | |||
| const plugins = require(`./plugins/best-match`) | |||
| const modulePlugins = require('./plugins/module'); | |||
There was a problem hiding this comment.
Perhaps rename the file to modules, and import it into a determineModulesPlugin() variable?
plugins/module.js
Outdated
| @@ -0,0 +1,11 @@ | |||
| 'use strict'; | |||
|
|
|||
| module.exports = function (opts) { | |||
There was a problem hiding this comment.
Let's stay consistent and name this options.
plugins/module.js
Outdated
| 'use strict'; | ||
|
|
||
| module.exports = function (opts) { | ||
| const modules = opts && opts.modules !== null ? opts.modules : true; |
There was a problem hiding this comment.
Can we treat this the same as https://github.com/babel/babel-preset-env#modules? E.g. the only allowed values are 'commonjs' or false.
plugins/module.js
Outdated
| throw new Error('@ava/stage-4 only support commonjs modules'); | ||
| } | ||
|
|
||
| return modules ? require('./commonjs.json') : []; |
There was a problem hiding this comment.
Feel free to return ['babel-plugin-transform-es2015-modules-commonjs'] directly. The JSON files are there so the plugin lists are easier to manage per Node.js version, but that doesn't apply here.
dinoboff
force-pushed
the
feat/modules
branch
3 times, most recently
from
ead7129 to
0ab8850
Compare
Only support transforming ES module to “commonjs” or to disable the module transformation. "babel-preset-env" supports other legacy module types which are not relevant for AVA.
dinoboff
force-pushed
the
feat/modules
branch
from
0ab8850 to
4caaac2
Compare
|
@novemberborn Can you review the changes? |
|
|
||
| const plugins = require('./plugins/best-match') | ||
| const plugins = Object.keys(determineModulesPlugin.supported) | ||
| .map(k => determineModulesPlugin.supported[k]) |
There was a problem hiding this comment.
Just starts with [babel-plugin-transform-es2015-modules-commonjs] and run .concat(require('./plugins/best-match') on that.
| throw new Error('@ava/stage-4 only supports commonjs module transformation.'); | ||
| } | ||
|
|
||
| return [plugin]; |
There was a problem hiding this comment.
I'd say this is clearer:
if (!options || options.modules === false) {
return [];
}
if (options.modules !== 'commonjs') {
throw new Error('@ava/stage-4 only supports commonjs module transformation.');
}
return ['babel-plugin-transform-es2015-modules-commonjs'];There was a problem hiding this comment.
Does babel always provides options, even when the preset is set without any?
There was a problem hiding this comment.
Ah no, my bad. We should default to commonjs if there are no options.
|
|
||
| `"commonjs" | false`, defaults to `"commonjs"`. | ||
|
|
||
| Enable transformation of ES6 module syntax to another module type. |
There was a problem hiding this comment.
I'd prefer ES2015 over ES6. That also corresponds with the plugin package name.
There was a problem hiding this comment.
ok, it was taken from env preset.
Only support transforming ES module to “commonjs” or to disable the module transformation. "babel-preset-env" supports other legacy module types which are not relevant for AVA.