Skip to content
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

Disallows specifiers after export * as ns #15385

Merged

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jan 30, 2023

Q                       A
Fixed Issues? Disallows export * as ns, { foo } from "module"
Patch: Bug Fix? Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

The current ExportFromClause production

ExportFromClause :
  *
  * as ModuleExportName
  NamedExports

does not allow named exports after a namespace export, i.e.

export * as ns, { foo } from "module"

is invalid. Currently they are allowed (long since the babylon era). I don't know if it reflects on an earlier version of the export-ns-from proposal but we have to disallow it from now on to align with the spec. Besides, my local experiment reveal that V8, SM and Webkit all disallow such production, so it is very likely not a spec oversight.

The test cases are updated accordingly.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories pkg: parser labels Jan 30, 2023
export * as default from "foo";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ExportSpecifier4 is removed, then I renamed ExportSpecifier16 into ExportSpecifier4 to fill the hole.

@babel-bot
Copy link
Collaborator

babel-bot commented Jan 30, 2023

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

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 30, 2023

/cc @fisker The e2e tests are failing, for good reasons.

@fisker

This comment was marked as outdated.

@fisker

This comment was marked as outdated.

@fisker
Copy link
Contributor

fisker commented Jan 31, 2023

I was stupid, you can just

rm tests/format/js/exports/jsfmt.spec.js

@JLHwung
Copy link
Contributor Author

JLHwung commented Jan 31, 2023

tests/format/js/exports/jsfmt.spec.js

Thanks, hopefully we will release this patch soon and reenable this test.

Co-authored-by: fisker Cheung <lionkay@gmail.com>
@JLHwung JLHwung merged commit 3aee4e3 into babel:main Feb 2, 2023
@JLHwung JLHwung deleted the fix-invalid-export-from-clause-production branch February 2, 2023 14:57
coderaiser added a commit to putoutjs/recast that referenced this pull request Feb 12, 2023
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 5, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: parser PR: Spec Compliance 👓 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants