Skip to content

ExportNamedDeclaration always has attributes property#1346

Merged
marijnh merged 1 commit intoacornjs:masterfrom
overlookmotel:export-named
Mar 5, 2025
Merged

ExportNamedDeclaration always has attributes property#1346
marijnh merged 1 commit intoacornjs:masterfrom
overlookmotel:export-named

Conversation

@overlookmotel
Copy link
Contributor

@overlookmotel overlookmotel commented Mar 4, 2025

ESTree spec for ES2025 states about ExportNamedDeclaration:

attributes must be an empty array when source is null.

Acorn's AST currently does not include an attributes field at all when source is null.

This PR fixes that, by adding an attributes: [] property in this case.

This may improve performance slightly in consuming code, as consistent object shape across all ExportNamedDeclaration nodes allows the JS engine to monomorphize functions that deal in these nodes.

@RReverser
Copy link
Member

Looks reasonable. Can you add a regression test please?

graphite-app bot pushed a commit to oxc-project/oxc that referenced this pull request Mar 5, 2025
…tributes` field (#9546)

Acorn does not include an `attributes` field on `ExportNamedDeclaration` when `source == null`. I think that's an oversight - acornjs/acorn#1346.

Bump `acorn-test262` to include commit oxc-project/estree-conformance@96faefa which adds the missing `attributes` field to Acorn's AST.

This allows simplifying serialization/deserialization code, as we now don't need to deal with this special case.
@overlookmotel
Copy link
Contributor Author

Thanks for coming back so swiftly.

Have added tests. They don't cover every possible form of ExportNamedDeclaration, but they do cover regression of the change made in this PR.

@marijnh marijnh merged commit 0daa290 into acornjs:master Mar 5, 2025
1 check passed
@marijnh
Copy link
Member

marijnh commented Mar 5, 2025

Thanks! Merged.

@overlookmotel overlookmotel deleted the export-named branch March 5, 2025 12:42
@overlookmotel
Copy link
Contributor Author

overlookmotel commented Mar 5, 2025

Thanks very much for merging my 3 PRs so swiftly. Really appreciate it.

If I can make a request... When you have time, could you possibly make a release including these changes?

@marijnh
Copy link
Member

marijnh commented Mar 5, 2025

Sure. I've tagged a version 8.14.1!

@overlookmotel
Copy link
Contributor Author

Sure. I've tagged a version 8.14.1!

Brilliant. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants