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

whatwg/tc39/node.js JSON modules #9246

Closed
dnalborczyk opened this issue Jun 10, 2019 · 4 comments
Closed

whatwg/tc39/node.js JSON modules #9246

dnalborczyk opened this issue Jun 10, 2019 · 4 comments

Comments

@dnalborczyk
Copy link
Contributor

dnalborczyk commented Jun 10, 2019

Bug report Change request

(this is not exactly a bug), more a change in behavior request to align with other implementations.

the expected change would be to be consistent with (see below) whatwg/tc39/node.js and treat a json module import as a default import, and do not allow or support named exports.

basically throw a warning, same or similar to what v4 does with js modules not having a named export, e.g.

WARNING in ./src/index.js 3:12-16
"export 'test' was not found in './data.json'

couple reasons for not considering named exports: confusing and non-consistent behavior of "JSON Object" vs "JSON Array" (+ others, "string", null etc.). Also how to handle a "default" property on an "JSON Object".

additional background:

WHATWG
whatwg/html#4315
https://html.spec.whatwg.org/#json-module-script
https://html.spec.whatwg.org/#creating-a-json-module-script (give it some time to load)

TC39 https://docs.google.com/presentation/d/1w8jWjD41htD7VxOejFqiHi6uGgHVWtZ_XmgFxgKkS7Q/edit#slide=id.g5947c7781f_0_20

Node.js modules working group
nodejs/node#27752

related Rollup issue:
rollup/rollup#2920

** Expected Behavior

// allow
import data from './data.json'

// disallow
import { foo } from './data.json'

this would be a great addition for v5, as it would be a breaking change. not sure about the feasibility of integrating the same in v4 since it's only a warning.

edit: added Rollup issue reference

@dnalborczyk
Copy link
Contributor Author

awesome! thanks @sokra !

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Oct 17, 2020

@sokra some additional information regarding JSON module imports:

the above mentioned [previous] proposal has been changed to: https://github.com/tc39/proposal-json-modules (currently stage 2), which was spun off from: https://github.com/tc39/proposal-import-assertions/ (stage 3)

allowing only the default import for JSON modules still stands, although the import will use an additional import assertion. I believe importing JSON without the assertion would not be allowed.

@sokra
Copy link
Member

sokra commented Oct 17, 2020

Nevertheless, the interpretation of module loads with no assertions remains host/implementation-defined, so it is valid to implement JSON modules without requiring assert { type: "json" }. It's just that assert { type: "json" } must be supported everywhere. For example, it will be up to Node.js, not TC39, to decide whether import assertions are required or optional for JSON modules.

Current it's optional. We will support the assert syntax once we can parse it (acorn)

@ljharb
Copy link

ljharb commented Apr 14, 2022

Importing JSON without the assertion is explicitly allowed, and imo encouraged.

lucasnetau added a commit to lucasnetau/formBuilder that referenced this issue Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants