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

JSON modules should be imported with default #14668

Merged
merged 8 commits into from Jun 23, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jun 14, 2022

Q                       A
Fixed Issues? Disallow import { foo } from "./data.json" assert { type: "json" }
Patch: Bug Fix? Y
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Per https://github.com/tc39/proposal-json-modules#why-dont-json-modules-support-named-exports, JSON modules does not support named exports so we should disallow such patterns.

@JLHwung JLHwung added PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Import Attributes labels Jun 14, 2022
| N.ImportDeclaration,
): boolean {
if (node.assertions != null) {
return node.assertions.some(({ key, value }) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently import assertions only allow type: "json", so isJSONModuleImport always return true. If more import assertions are supported, then we should differentiate each assertions/attributes.

Copy link
Member

Choose a reason for hiding this comment

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

We can add a test where node.assertions is not null but it does not return true: import { foo } from "bar" asserts {};

@@ -1 +1 @@
export { foo } from "foo.json" assert { type: "json", hasOwnProperty: "true" };
export { default } from "foo.json" assert { type: "json", hasOwnProperty: "true" };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not related to this PR: I think this case should have been forbidden.

@babel-bot
Copy link
Collaborator

babel-bot commented Jun 14, 2022

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

@@ -1 +0,0 @@
export { foo } from "foo.json" assert { type: "json", type: "html" };
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to invalid-syntax-export-with-repeated-type/input.js

@JLHwung JLHwung force-pushed the import-default-json-modules branch from 8835efd to 669b4bb Compare June 14, 2022 18:18
@liuxingbaoyu
Copy link
Member

Is this a breaking change?

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 14, 2022

Yes. The spec compliance PRs are potentially breaking changes for some early adopters, but we generally ship them in patch release since Babel's goal is to be as compliant to the spec as possible.

@liuxingbaoyu
Copy link
Member

I fear there will be no small impact, like our e2e failure.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 14, 2022

The e2e test is failing because prettier is testing named JSON module import, which is invalid according to the spec. Since JSON module is a stage 3 proposal and I would expect few actual usage, @sosukesuzuki Can prettier update the snapshot accordingly after this PR is released?

@nicolo-ribaudo
Copy link
Member

Is the fact that https://github.com/nicolo-ribaudo/test-import-json-node logs { a: 2, b: 3, default: { a: 2, b: 3 } } a Node.js bug that skips some checks with loaders (tested with Node.js 18.3)?

@nicolo-ribaudo
Copy link
Member

Ok yes, it's definitely a Node.js bug.

I agree that this is a bugfix, but it feels risky: there might already be some people relying on our behavior (which contradicts the spec), so I would not be surprised if we'll have to revert and defer to Babel 8.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 16, 2022

I agree it is a Node.js bug, the JSON modules docs states that named exports are not supported. I think Node.js should check the module records after applying the custom loader to a JSON module.

Note that Node.js 18.3 already throws for import { name } from "./data.json" assert { type: "json" }, so we are aligning to the Node.js behaviour.

@liuxingbaoyu
Copy link
Member

This PR looks good to me. But I hope we can merge it when CI is green.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jun 20, 2022

Currently pending prettier/prettier#13031.

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

I was initially unsure about this, since it feels like it should be checked at "linking time" and not at "parsing time". However, we cannot really differentiate between them.

I'm ok with merging without waiting for the prettier PR.

packages/babel-parser/src/parse-error/standard-errors.js Outdated Show resolved Hide resolved
| N.ImportDeclaration,
): boolean {
if (node.assertions != null) {
return node.assertions.some(({ key, value }) => {
Copy link
Member

Choose a reason for hiding this comment

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

We can add a test where node.assertions is not null but it does not return true: import { foo } from "bar" asserts {};

@JLHwung JLHwung force-pushed the import-default-json-modules branch from f5963e2 to 09b8b33 Compare June 21, 2022 15:10
@@ -0,0 +1 @@
import "foo" assert {};
Copy link
Member

Choose a reason for hiding this comment

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

I meant specifically, import { x } from "foo" assert {} shouldn't be an error because even if we have assertions they don't contain type: "json".

@liuxingbaoyu
Copy link
Member

It looks like we need to rename the test.

@JLHwung JLHwung merged commit e8dce41 into babel:main Jun 23, 2022
@JLHwung JLHwung deleted the import-default-json-modules branch June 23, 2022 13:30
@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 Sep 23, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2022
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 Spec: Import Attributes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants