Repository navigation
perf: Improve @babel/parser - #18176
Conversation
| // TODO: Enable this | ||
| checkNamedExport(): void { | ||
| // const { specifiers } = node; | ||
| // if (specifiers?.length) { | ||
| // node.specifiers = specifiers.filter( | ||
| // // @ts-expect-error placeholder typings | ||
| // node => node.exported.type === "Placeholder", | ||
| // ); | ||
| // } | ||
| // super.checkNamedExport(node); | ||
| // node.specifiers = specifiers; | ||
| } |
There was a problem hiding this comment.
Commenting them out won't change the behavior because we didn't pass any parameters when calling the parent class function in the past, so there was no actual check.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/61997 |
| const attachComment = (this.optionFlags & OptionFlags.AttachComment) > 0; | ||
| let comments: N.Comment[] | undefined; |
There was a problem hiding this comment.
This is the main reason for the performance improvement, now we only create arrays when needed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7405da9832
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.checkDuplicateExports(node, "default"); | ||
| if (this.hasPlugin("exportDefaultFrom")) { |
There was a problem hiding this comment.
Preserve placeholder handling for default-export validation
When the placeholders plugin is enabled, default-export validation now bypasses the plugin hook that previously suppressed these checks. For example, with placeholders and exportDefaultFrom, export default from; was previously accepted as exporting an identifier named from, but the inlined ExportDefaultFromAsIdentifier check now rejects it; similarly, templates containing multiple provisional default exports begin throwing duplicate-export errors. Keep these checks behind an overridable hook, as is still done for named exports, to avoid changing placeholder-template parsing in this performance/type-only change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I investigated.
This resulted in some behavioral changes, but they seem acceptable.
This used to be legal, now it throws an error.
template(`
export default %%FIRST%%;
export default %%SECOND%%;
`);This now throws an error when exportDefaultFrom and placeholders are enabled.
Ideally, the behavior should throw an error as soon as exportDefaultFrom is enabled, and should not be affected by placeholders. The new behavior is better.
export default from;| ); | ||
| } | ||
|
|
||
| parseTypeOnlyExportSpecifier( |
There was a problem hiding this comment.
I split parseTypeOnlyImportExportSpecifier into two functions, which may result in a slight increase in size, but I feel that two functions are clearer and may be more efficient, so it's acceptable.
| UsingDeclarationInAmbientContext: (kind: "using" | "await using") => | ||
| `'${kind}' declarations are not allowed in ambient contexts.`, | ||
| } satisfies ParseErrorTemplates; | ||
| /* eslint-disable sort-keys */ |
There was a problem hiding this comment.
This wasn't actually working as we intended, it had previously only suppressed reporting throughout the entire file. Now that there's no code violating this rule, it will be treated as an error by ESLint.
|
commit: |
Fixes #1, Fixes #2This PR mainly focuses on some type improvements.