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 👓 Spec: Import Assertions labels Jun 14, 2022
| N.ImportDeclaration,
): boolean {
if (node.assertions != null) {
return node.assertions.some(({ key, value }) => {
Copy link
Contributor Author

@JLHwung JLHwung Jun 14, 2022

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

@nicolo-ribaudo nicolo-ribaudo Jun 20, 2022

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

@JLHwung JLHwung Jun 14, 2022

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

@JLHwung JLHwung Jun 14, 2022

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 Jun 14, 2022
@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented Jun 14, 2022

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
Contributor

liuxingbaoyu commented Jun 14, 2022

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

nicolo-ribaudo commented Jun 16, 2022

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

nicolo-ribaudo commented Jun 16, 2022

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
Contributor

liuxingbaoyu commented Jun 16, 2022

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

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

@nicolo-ribaudo nicolo-ribaudo Jun 20, 2022

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 Jun 21, 2022
@@ -0,0 +1 @@
import "foo" assert {};
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jun 21, 2022

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
Contributor

liuxingbaoyu commented Jun 21, 2022

It looks like we need to rename the test.

@JLHwung JLHwung merged commit e8dce41 into babel:main Jun 23, 2022
35 checks passed
@JLHwung JLHwung deleted the import-default-json-modules branch Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: parser PR: Spec Compliance 👓 Spec: Import Assertions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants