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

Enable import attributes parsing by default #16850

Merged
merged 30 commits into from
Oct 24, 2024

Conversation

nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Sep 23, 2024

Q                       A
Fixed Issues? Fixes #1, Fixes #2
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link babel/website#2974
Any Dependency Changes?
License MIT

It got consensus for Stage 4 at the October 2024 TC39 meeting.

This PR adds a "deprecatedImportAssert" parser plugin, as a replacement for ["importAttributes", { "depreatedAssertSyntax": true }] so that we can remove the importAttributes plugin (because it won't be needed anymore).

This PR also has some changes relevant for Babel 8:

  • Using the importAssertions parser plugin now throws, as it's removed and not supported anymore
  • Using the [importAttributes, { deprecatedAssertSyntax: true }] parser plugin now throws, telling you to use deprecatedImportAssert instead (unless you are already using that plugin, in which case we simply ignore the old one)
  • @babel/generator defaults to printing attributes as with { x: "foo" } instead of with x: "foo".

@nicolo-ribaudo nicolo-ribaudo added the PR: New Feature 🚀 A type of pull request used for our changelog categories label Sep 23, 2024
@nicolo-ribaudo nicolo-ribaudo added this to the v7.26.0 milestone Sep 23, 2024
@babel-bot
Copy link
Collaborator

babel-bot commented Sep 23, 2024

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

@nicolo-ribaudo
Copy link
Member Author

This reached consensus for stage 4.

@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch from 4b411ae to c81d348 Compare October 9, 2024 12:25
@@ -8,6 +8,7 @@ async_arrow_functions/with_type_parameters_types_disabled.js
class_method_kinds/polymorphic_getter.js
class_properties/migrated_0026.js
comment_interning/declare_variable.js
dynamic_import/migrated_0005.js
Copy link
Member Author

Choose a reason for hiding this comment

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

This is testing that dynamic import does not accept a second argument

@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch from fecbca9 to fc0e5cd Compare October 9, 2024 14:24
@JLHwung
Copy link
Contributor

JLHwung commented Oct 22, 2024

Aside: Now that JSON module also reaches stage 4, should we rename proposal-json-modules and add it to preset-env?

@nicolo-ribaudo
Copy link
Member Author

We should definitely rename it, but I'm a bit hesitant about adding it by default to preset-env because it can change existing behavior when transforming modules.

For example, if you have syntax-import-attribtues enabled, the modules transform will convert import "./foo.json" with { type: "json" } to just require("./foo.json"). However, if you enable proposal-json-modules then it will transform to a fs.readFileSync call.

Maybe we could enable it only when not compiling modules?

@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch 3 times, most recently from 0f16dab to e5d28bd Compare October 22, 2024 15:31
@nicolo-ribaudo
Copy link
Member Author

One day CI for this PR will be green, I promise.

@nicolo-ribaudo nicolo-ribaudo added the PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release label Oct 23, 2024
@nicolo-ribaudo
Copy link
Member Author

CI is finally green. I updated the PR description to reflect the current state of the PR :)

@nicolo-ribaudo nicolo-ribaudo added the PR: Needs Review A pull request awaiting more approvals label Oct 23, 2024
@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch from 4de6ca1 to 6230a31 Compare October 23, 2024 14:27
@nicolo-ribaudo
Copy link
Member Author

Ughh rebasing broke it

@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch from 90803dc to dacba13 Compare October 23, 2024 14:57
@nicolo-ribaudo
Copy link
Member Author

The failure is a bug in Babel that, when fixed, triggers a bug in Node.js. I'll open a PR with a workaround soon.

@@ -1,3 +1,3 @@
{
"throws": "This experimental syntax requires enabling the parser plugin: \"importAttributes\". (1:16)"
"throws": "Unexpected token, expected \")\" (1:25)"
Copy link
Member

@liuxingbaoyu liuxingbaoyu Oct 24, 2024

Choose a reason for hiding this comment

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

It should throw an ImportCallArity?

@@ -0,0 +1,31 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the file name or the input.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can be just removed 👍

@nicolo-ribaudo nicolo-ribaudo force-pushed the import-attributes-default branch from e99cefe to d8026e4 Compare October 24, 2024 14:51
@nicolo-ribaudo nicolo-ribaudo merged commit 64fa466 into babel:main Oct 24, 2024
54 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the import-attributes-default branch October 24, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: parser PR: Breaking Change 💥 A type of pull request used for our changelog categories for next major release PR: Needs Review A pull request awaiting more approvals PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants