JSON modules should be imported with default#14668
Conversation
| | N.ImportDeclaration, | ||
| ): boolean { | ||
| if (node.assertions != null) { | ||
| return node.assertions.some(({ key, value }) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" }; | |||
There was a problem hiding this comment.
Not related to this PR: I think this case should have been forbidden.
|
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" }; | |||
There was a problem hiding this comment.
Renamed to invalid-syntax-export-with-repeated-type/input.js
8835efd to
669b4bb
Compare
|
Is this a breaking change? |
|
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. |
|
I fear there will be no small impact, like our e2e failure. |
|
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? |
|
Is the fact that https://github.com/nicolo-ribaudo/test-import-json-node logs |
|
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. |
|
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 |
|
This PR looks good to me. But I hope we can merge it when CI is green. |
|
Currently pending prettier/prettier#13031. |
nicolo-ribaudo
left a comment
There was a problem hiding this 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.
| | N.ImportDeclaration, | ||
| ): boolean { | ||
| if (node.assertions != null) { | ||
| return node.assertions.some(({ key, value }) => { |
There was a problem hiding this comment.
We can add a test where node.assertions is not null but it does not return true: import { foo } from "bar" asserts {};
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
f5963e2 to
09b8b33
Compare
| @@ -0,0 +1 @@ | |||
| import "foo" assert {}; | |||
There was a problem hiding this comment.
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".
|
It looks like we need to rename the test. |
import { foo } from "./data.json" assert { type: "json" }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.