-
-
Notifications
You must be signed in to change notification settings - Fork 615
feat(babel)!: add parallel processing via worker threads #1956
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -1,14 +1,37 @@ | ||||
| import { readFileSync } from 'fs'; | ||||
|
|
||||
| import { createConfig } from '../../shared/rollup.config.mjs'; | ||||
| import { createConfig, emitModulePackageFile } from '../../shared/rollup.config.mjs'; | ||||
|
|
||||
| import { babel } from './src/index.js'; | ||||
|
|
||||
| const pkg = JSON.parse(readFileSync(new URL('./package.json', import.meta.url), 'utf8')); | ||||
|
|
||||
| export default { | ||||
| ...createConfig({ pkg }), | ||||
| input: './src/index.js', | ||||
| input: { | ||||
| index: './src/index.js', | ||||
| worker: './src/worker.js' | ||||
| }, | ||||
| output: [ | ||||
| { | ||||
| format: 'cjs', | ||||
| dir: 'dist/cjs', | ||||
| exports: 'named', | ||||
| footer(chunkInfo) { | ||||
| if (chunkInfo.name === 'index') { | ||||
| return 'module.exports = Object.assign(exports.default, exports);'; | ||||
| } | ||||
| return null; | ||||
|
Comment on lines
+20
to
+24
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this now? I don't see any change in the index.js that would affect the package's shape
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had to refactor plugins/shared/rollup.config.mjs Line 28 in 7d16103
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: have you tested the plugin works now correctly in both CJS and ESM modes? |
||||
| }, | ||||
| sourcemap: true | ||||
| }, | ||||
| { | ||||
| format: 'es', | ||||
| dir: 'dist/es', | ||||
| plugins: [emitModulePackageFile()], | ||||
| sourcemap: true | ||||
| } | ||||
| ], | ||||
| plugins: [ | ||||
| babel({ | ||||
| presets: [['@babel/preset-env', { targets: { node: 14 } }]], | ||||
|
|
||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,10 +2,9 @@ import * as babel from '@babel/core'; | |
| import { createFilter } from '@rollup/pluginutils'; | ||
|
|
||
| import { BUNDLED, HELPERS } from './constants.js'; | ||
| import bundledHelpersPlugin from './bundledHelpersPlugin.js'; | ||
| import preflightCheck from './preflightCheck.js'; | ||
| import transformCode from './transformCode.js'; | ||
| import { addBabelPlugin, escapeRegExpCharacters, warnOnce, stripQuery } from './utils.js'; | ||
| import { escapeRegExpCharacters, warnOnce } from './utils.js'; | ||
| import WorkerPool from './workerPool.js'; | ||
|
|
||
| const unpackOptions = ({ | ||
| extensions = babel.DEFAULT_EXTENSIONS, | ||
|
|
@@ -37,7 +36,7 @@ const warnAboutDeprecatedHelpersOption = ({ deprecatedOption, suggestion }) => { | |
| ); | ||
| }; | ||
|
|
||
| const unpackInputPluginOptions = ({ skipPreflightCheck = false, ...rest }, rollupVersion) => { | ||
| const unpackInputPluginOptions = ({ skipPreflightCheck = false, ...rest }) => { | ||
| if ('runtimeHelpers' in rest) { | ||
| warnAboutDeprecatedHelpersOption({ | ||
| deprecatedOption: 'runtimeHelpers', | ||
|
|
@@ -63,8 +62,7 @@ const unpackInputPluginOptions = ({ skipPreflightCheck = false, ...rest }, rollu | |
| supportsStaticESM: true, | ||
| supportsDynamicImport: true, | ||
| supportsTopLevelAwait: true, | ||
| // todo: remove version checks for 1.20 - 1.25 when we bump peer deps | ||
| supportsExportNamespaceFrom: !rollupVersion.match(/^1\.2[0-5]\./), | ||
| supportsExportNamespaceFrom: true, | ||
| ...rest.caller | ||
| } | ||
| }); | ||
|
|
@@ -101,6 +99,24 @@ const returnObject = () => { | |
| return {}; | ||
| }; | ||
|
|
||
| function isSerializable(value) { | ||
| if (value === null) { | ||
| return true; | ||
| } else if (Array.isArray(value)) { | ||
| return value.every(isSerializable); | ||
| } | ||
| switch (typeof value) { | ||
| case 'string': | ||
| case 'number': | ||
| case 'boolean': | ||
| return true; | ||
| case 'object': | ||
| return Object.keys(value).every((key) => isSerializable(value[key])); | ||
| default: | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| function createBabelInputPluginFactory(customCallback = returnObject) { | ||
| const overrides = customCallback(babel); | ||
|
|
||
|
|
@@ -110,77 +126,113 @@ function createBabelInputPluginFactory(customCallback = returnObject) { | |
| overrides | ||
| ); | ||
|
|
||
| let babelHelpers; | ||
| let babelOptions; | ||
| let filter; | ||
| let skipPreflightCheck; | ||
| return { | ||
| name: 'babel', | ||
|
|
||
| options() { | ||
| // todo: remove options hook and hoist declarations when version checks are removed | ||
| let exclude; | ||
| let include; | ||
| let extensions; | ||
| let customFilter; | ||
| let workerPool; | ||
|
|
||
| const { | ||
| exclude, | ||
| extensions, | ||
| babelHelpers, | ||
| include, | ||
| filter: customFilter, | ||
| skipPreflightCheck, | ||
| parallel, | ||
| ...babelOptions | ||
| } = unpackInputPluginOptions(pluginOptionsWithOverrides); | ||
|
|
||
| const extensionRegExp = new RegExp( | ||
| `(${extensions.map(escapeRegExpCharacters).join('|')})(\\?.*)?(#.*)?$` | ||
| ); | ||
| if (customFilter && (include || exclude)) { | ||
| throw new Error('Could not handle include or exclude with custom filter together'); | ||
| } | ||
| const userDefinedFilter = | ||
| typeof customFilter === 'function' ? customFilter : createFilter(include, exclude); | ||
| const filter = (id, code) => extensionRegExp.test(id) && userDefinedFilter(id, code); | ||
|
|
||
| ({ | ||
| exclude, | ||
| extensions, | ||
| babelHelpers, | ||
| include, | ||
| filter: customFilter, | ||
| skipPreflightCheck, | ||
| ...babelOptions | ||
| } = unpackInputPluginOptions(pluginOptionsWithOverrides, this.meta.rollupVersion)); | ||
| if (parallel) { | ||
| const parallelAllowed = | ||
| isSerializable(babelOptions) && !overrides?.config && !overrides?.result; | ||
|
Comment on lines
+152
to
+154
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. perhaps it would be nice to move this validation to |
||
|
|
||
| const extensionRegExp = new RegExp( | ||
| `(${extensions.map(escapeRegExpCharacters).join('|')})$` | ||
| if (!parallelAllowed) { | ||
| throw new Error( | ||
| 'Cannot use "parallel" mode alongside custom overrides or non-serializable Babel options.' | ||
| ); | ||
| if (customFilter && (include || exclude)) { | ||
| throw new Error('Could not handle include or exclude with custom filter together'); | ||
| } | ||
| const userDefinedFilter = | ||
| typeof customFilter === 'function' ? customFilter : createFilter(include, exclude); | ||
| filter = (id) => extensionRegExp.test(stripQuery(id).bareId) && userDefinedFilter(id); | ||
| } | ||
|
|
||
| const parallelWorkerCount = typeof parallel === 'number' ? parallel : 4; | ||
| workerPool = new WorkerPool( | ||
| new URL('./worker.js', import.meta.url).pathname, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this compatible with repo-wide supported node.js version? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would ci tell us?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worker Threads and |
||
| parallelWorkerCount | ||
| ); | ||
| } | ||
|
|
||
| return null; | ||
| const helpersFilter = { id: new RegExp(`^${escapeRegExpCharacters(HELPERS)}$`) }; | ||
|
|
||
| return { | ||
| name: 'babel', | ||
|
|
||
| resolveId: { | ||
| filter: helpersFilter, | ||
| handler(id) { | ||
| if (id !== HELPERS) { | ||
| return null; | ||
| } | ||
| return id; | ||
| } | ||
| }, | ||
|
|
||
| resolveId(id) { | ||
| if (id !== HELPERS) { | ||
| return null; | ||
| load: { | ||
| filter: helpersFilter, | ||
| handler(id) { | ||
| if (id !== HELPERS) { | ||
| return null; | ||
| } | ||
| return babel.buildExternalHelpers(null, 'module'); | ||
| } | ||
| return id; | ||
| }, | ||
|
|
||
| load(id) { | ||
| if (id !== HELPERS) { | ||
| return null; | ||
| transform: { | ||
| filter: { | ||
| id: extensionRegExp | ||
| }, | ||
| async handler(code, filename) { | ||
| if (!(await filter(filename, code))) return null; | ||
| if (filename === HELPERS) return null; | ||
|
|
||
| if (parallel) { | ||
| return workerPool.runTask({ | ||
| inputCode: code, | ||
| babelOptions: { ...babelOptions, filename }, | ||
| runPreflightCheck: !skipPreflightCheck, | ||
| babelHelpers | ||
| }); | ||
|
Comment on lines
+203
to
+208
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: I'm not sure what we actually get from |
||
| } | ||
|
|
||
| return transformCode({ | ||
| inputCode: code, | ||
| babelOptions: { ...babelOptions, filename }, | ||
| overrides: { | ||
| config: overrides.config?.bind(this), | ||
| result: overrides.result?.bind(this) | ||
| }, | ||
| customOptions, | ||
| error: this.error.bind(this), | ||
| runPreflightCheck: !skipPreflightCheck, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMHO, this conversion make the code slightly harder to follow. We should standardize on a single variable name for this |
||
| babelHelpers | ||
| }); | ||
| } | ||
| return babel.buildExternalHelpers(null, 'module'); | ||
| }, | ||
|
|
||
| transform(code, filename) { | ||
| if (!filter(filename)) return null; | ||
| if (filename === HELPERS) return null; | ||
| async closeBundle() { | ||
| if (parallel && !this.meta.watchMode) { | ||
| await workerPool.terminate(); | ||
| } | ||
| }, | ||
|
|
||
| return transformCode( | ||
| code, | ||
| { ...babelOptions, filename }, | ||
| overrides, | ||
| customOptions, | ||
| this, | ||
| async (transformOptions) => { | ||
| if (!skipPreflightCheck) { | ||
| await preflightCheck(this, babelHelpers, transformOptions); | ||
| } | ||
|
|
||
| return babelHelpers === BUNDLED | ||
| ? addBabelPlugin(transformOptions, bundledHelpersPlugin) | ||
| : transformOptions; | ||
| } | ||
| ); | ||
| async closeWatcher() { | ||
| if (parallel) { | ||
| await workerPool.terminate(); | ||
| } | ||
| } | ||
| }; | ||
| }; | ||
|
|
@@ -259,7 +311,16 @@ function createBabelOutputPluginFactory(customCallback = returnObject) { | |
| } | ||
| } | ||
|
|
||
| return transformCode(code, babelOptions, overrides, customOptions, this); | ||
| return transformCode({ | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. q: why we don't attempt to parallelize this one? |
||
| inputCode: code, | ||
| babelOptions, | ||
| overrides: { | ||
| config: overrides.config?.bind(this), | ||
| result: overrides.result?.bind(this) | ||
| }, | ||
| customOptions, | ||
| error: this.error.bind(this) | ||
| }); | ||
| } | ||
| }; | ||
| }; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shellscape this required a major bump of this package - we should ensure that the automation will be able to bump this properly. I think that's achieved using some special tags in commit messages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah including
BREAKING CHANGESin the commit body or changing the PR title to reflect a major version in the conventional commit spec.