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
Conversation
| N.ImportDeclaration, | ||
): boolean { | ||
if (node.assertions != null) { | ||
return node.assertions.some(({ key, value }) => { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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" }; |
There was a problem hiding this comment.
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.
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.
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
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. |
There was a problem hiding this 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.
| N.ImportDeclaration, | ||
): boolean { | ||
if (node.assertions != null) { | ||
return node.assertions.some(({ key, value }) => { |
There was a problem hiding this comment.
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 {};
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.
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"
.
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.