[babel 8] Move ESLint parsing to a Worker #13199

nicolo-ribaudo · 2021-04-24T15:23:55Z

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

One of the main blockers for moving to ESM is that ESLint doesn't support asynchronous parsers yet (ref: eslint/eslint#14295).

This PR works around the problem by moving @babel/core usage inside @babel/eslint-parser inside a service worker: by doing that it doesn't matter that loading @babel/core is asynchronous, because we can always communicate with the worker synchronously. This can also be easily extended to allow users to use babel.config.mjs files with @babel/eslint-parser (currently it throws an error).

This PR touches all the files in @babel/eslint-parser, but the only new things are src/client.cjs, src/worker/index.cjs and src/worker/babel-core.cjs. Everything else is just moved around.

(EDIT: See #13199 (comment)) This PR causes (when BABEL_8_BREAKING is true) a performance drop of 10% when linting our repo with eslint-plugin-prettier disabled (5% when it's enabled). For this reason, I'll follow up with a different approach: rather than passing the input source code to the worker and getting back the AST (which is big and expensive to clone across workers), I want to only give the worker the input filename and make it return the set of options that we need to give to @babel/parser. By doing so, we can keep config/plugins resolution async while avoiding passing the big AST object around. However, this

requires @babel/parser to be CJS (and imo it would be acceptable, since it's also used as a standalone tool by other projects such as Prettier)
breaks tokTypes identity, so it can only be done after fixing @babel/core^7.13.10 eslint error: Cannot read property 'value' of null when @babel/types are duplicated in node_modules #12985

babel-bot · 2021-04-24T15:26:55Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/46140/

codesandbox-ci · 2021-04-24T15:27:53Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 3febba7:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

nicolo-ribaudo · 2021-05-07T10:45:15Z

I moved parsing from the worker to the main thread when using the default @babel/parser: by doing so we don't have to pass the whole AST between workers, but only the parser options.
When using a custom parser we still parse in the worker thread, since we cannot pass the parser function to the main thread (tested by https://github.com/babel/babel/blob/main/eslint/babel-eslint-tests/test/integration/duplicated-babel-parser.js).

The negative performance impact of using a separate worker is gone: running time yarn lint on main gives 26,32s user 0,85s system 143% cpu 18,874 total, while running time BABEL_8_BREAKING=true yarn lint on this PR gives 26,10s user 0,99s system 150% cpu 18,006 total/26,41s user 0,87s system 151% cpu 17,990 total. These measurements are done without eslint-plugin-prettier, since it makes ESLint way slower hiding the parser performance effects.

I converted the files to .cjs since when moving to native ESM we'll want to keep these as CJS, otherwise they won't work with ESLint.

The last commit isn't strictly required, but since this PR makes it easy to load the config asynchronously I removed the restriction that disallows .mjs config files when using @babel/eslint-parser. Since it's a new feature, we could decide to enable it behind an option in Babel 7 (but it can be done in a separate PR).

eslint/babel-eslint-parser/src/configuration.js

JLHwung · 2021-05-07T14:26:34Z

eslint/babel-eslint-parser/src/worker/configuration.cjs

+
+  if (process.env.BABEL_8_BREAKING) {
+    return babel
+      .loadPartialConfigAsync(parseOptions)


Should we load partial config when options.requireConfigFile is false?

I tried to match the current behavior, and I think that when requireConfigFile: false it still loads the config file if it's present, but it doesn't throw if it's not. To disable config file loading, you need to pass babelOptions: { configFile: false }.

eslint/babel-eslint-parser/src/worker/index.cjs

eslint/babel-eslint-parser/src/client.cjs

kaicataldo · 2021-05-10T17:05:33Z

eslint/babel-eslint-parser/src/client.cjs

+
+let send;
+
+exports.getVersion = sendCached("GET_VERSION");


Could we extract these action types to a constant that we reuse everywhere so we don't risk bugs due to typos?

I moved them to this file (rather than always using the string) so that we only have to care about those strings in the two "communication files" (worker client and server). Everywhere else we use getVersion() etc.
Also, https://github.com/babel/babel/pull/13199/files#diff-62c6b0f19ec3e004fffbecf4acab02bc005e538ea3c08e9bd558def1f28634c7R32 throws so that we catch any invalid string early.

kaicataldo

🎉

nicolo-ribaudo · 2021-05-17T15:04:23Z

I'll try again moving to esm and let's see how far I can get this time 🤞

nicolo-ribaudo added area: eslint babel-8-dev → main labels Apr 24, 2021

nicolo-ribaudo force-pushed the babel-8-eslint-parser-worker branch from ad943ff to e76b293 Compare May 7, 2021 10:44

JLHwung reviewed May 7, 2021

View reviewed changes

nicolo-ribaudo force-pushed the babel-8-eslint-parser-worker branch from 374d10e to 45f120b Compare May 10, 2021 16:45

kaicataldo reviewed May 10, 2021

View reviewed changes

JLHwung approved these changes May 14, 2021

View reviewed changes

nicolo-ribaudo added 6 commits May 15, 2021 00:09

[babel 8] Move ESLint parsing to a Worker

bd22b12

Parse in the main thread when using the default parser

38103e8

Support babel.config.mjs in @babel/eslint-parser

4b7284f

Review

525a8f0

Also lint with @babel/eslint-parser 8 in tests

fd7dfda

Add back trailing commas

3febba7

nicolo-ribaudo force-pushed the babel-8-eslint-parser-worker branch from b597589 to 3febba7 Compare May 14, 2021 22:10

kaicataldo approved these changes May 17, 2021

View reviewed changes

nicolo-ribaudo merged commit 9d620c2 into babel:main May 17, 2021

nicolo-ribaudo deleted the babel-8-eslint-parser-worker branch May 17, 2021 15:04

JLHwung mentioned this pull request May 18, 2021

[Bug]: Parsing error: Cannot read property 'push' of undefined #13331

Closed

1 task

nicolo-ribaudo mentioned this pull request May 21, 2021

[Bug]: @babel/eslint-parser doesn't work with an ESM babel config file #13352

Closed

1 task

merceyz mentioned this pull request May 23, 2021

chore(eslint-tests): add missing dependency @babel/core #13355

Merged

nicolo-ribaudo mentioned this pull request Jul 28, 2021

Expose @babel/eslint-parser/experimental-worker #13398

Merged

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Aug 17, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2021

nicolo-ribaudo added the babel 8 label Aug 8, 2023

nicolo-ribaudo removed the babel-8-dev → main label Aug 8, 2023

nicolo-ribaudo added this to the v8.0.0 milestone Aug 8, 2023

nicolo-ribaudo added PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release and removed babel 8 labels Aug 8, 2023

JLHwung added the PR: Needs Docs label Aug 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[babel 8] Move ESLint parsing to a Worker #13199

[babel 8] Move ESLint parsing to a Worker #13199

nicolo-ribaudo commented Apr 24, 2021 •

edited

Loading

babel-bot commented Apr 24, 2021 •

edited

Loading

codesandbox-ci bot commented Apr 24, 2021 •

edited

Loading

nicolo-ribaudo commented May 7, 2021 •

edited

Loading

JLHwung May 7, 2021

nicolo-ribaudo May 7, 2021 •

edited

Loading

kaicataldo May 10, 2021

nicolo-ribaudo May 10, 2021

kaicataldo left a comment

nicolo-ribaudo commented May 17, 2021


		let send;

		exports.getVersion = sendCached("GET_VERSION");

[babel 8] Move ESLint parsing to a Worker #13199

[babel 8] Move ESLint parsing to a Worker #13199

Conversation

nicolo-ribaudo commented Apr 24, 2021 • edited Loading

babel-bot commented Apr 24, 2021 • edited Loading

codesandbox-ci bot commented Apr 24, 2021 • edited Loading

nicolo-ribaudo commented May 7, 2021 • edited Loading

JLHwung May 7, 2021

Choose a reason for hiding this comment

nicolo-ribaudo May 7, 2021 • edited Loading

Choose a reason for hiding this comment

kaicataldo May 10, 2021

Choose a reason for hiding this comment

nicolo-ribaudo May 10, 2021

Choose a reason for hiding this comment

kaicataldo left a comment

Choose a reason for hiding this comment

nicolo-ribaudo commented May 17, 2021

nicolo-ribaudo commented Apr 24, 2021 •

edited

Loading

babel-bot commented Apr 24, 2021 •

edited

Loading

codesandbox-ci bot commented Apr 24, 2021 •

edited

Loading

nicolo-ribaudo commented May 7, 2021 •

edited

Loading

nicolo-ribaudo May 7, 2021 •

edited

Loading