-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
Make eslint-parser/experimental-worker the default
#17755
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
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60804 |
|
commit: |
nicolo-ribaudo
left a comment
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.
Nice
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.
This PR also removes the experimental worker. As far as I know, one thing that blocks the removal of experimental worker is that require(esm) does not support top level await but the worker implementation does support such usage. Is this issue resolved?
OK, it seems that this PR merges experimental-worker with the index. It should be documented in the migration notes then.
|
|
Users can use top level await in |
|
By the way, ESLint v10 is about to be released as a stable version. Do we plan to drop ESLint v8? |
|
Technically without the worker we don't support top-level await, but I suspect that that's fine. The motivation for the worker was originally to use ESM, I expect that top-level await in Babel configs is very rare and can be replaced with sync operations (since probably the only common I/O might be to read some config file somewhere). |
|
It may be a bit late to drop asynchronous support now, because our core has supported asynchronous operations for a long time, many plugins have already used asynchronous operations, and some third-party packages only provide asynchronous operations. I will investigate how much time it takes us to execute the entire configuration file in order to obtain the parser options. |
| if ( | ||
| options.babelOptions.babelrc === false && | ||
| options.babelOptions.configFile === false | ||
| ) { | ||
| const parserOpts = { | ||
| sourceType: options.sourceType, | ||
| sourceFilename: options.filePath, | ||
| ...(options.sourceType !== "commonjs" | ||
| ? { | ||
| allowReturnOutsideFunction: | ||
| options.ecmaFeatures?.globalReturn ?? false, | ||
| } | ||
| : {}), | ||
| ...options.babelOptions.parserOpts, | ||
| plugins: getParserPlugins(options.babelOptions), | ||
| // skip comment attaching for parsing performance | ||
| attachComment: false, | ||
| ranges: true, | ||
| tokens: true, | ||
| }; |
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.
The cost of worker threads is low, but I still added a fast path.
f045614 to
007f7bd
Compare
nicolo-ribaudo
left a comment
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.
I think this PR is fine, but if the default performance is worse we might want in the future to add a useSameThread: true option to opt-out from the worker and use sync Babel APIs.
|
Users can do this with the fast path now. :) |
|
I mean for when they still have a config, since in most cases everything will be fully sync anyway. Or we should at least document that if you have |
|
It seems most users only need to provide a list of parser plugins. By the way, I remember we used to want to disable the babelrc search by default in Babel 8. |
Before {
plugins: [
require("@babel/plugin-transform-class-properties")
]
}to {
plugins: [
await import("@babel/plugin-transform-class-properties")
]
}because it is easier to find-and-replace and users don't have to name every plugins again in static import. Now that |
Yes, but that requires duplicating your syntax configuration across your Babel config ans ESLint config. I just tested the performance of using vs not using the worker in our repository (by commenting out the |
|
To recap, since a couple of things changes since @JLHwung's approval:
|
Uh oh!
There was an error while loading. Please reload this page.