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

doc: remove esm no-json-module-loading section #40770

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnalborczyk
Copy link
Contributor

@dnalborczyk dnalborczyk commented Nov 10, 2021

with v17.1.0 loading json behind the --experimental-json-modules flag has been removed and requires import assertions behind the same flag.

ref: #40758 (comment)

@dnalborczyk dnalborczyk changed the title doc: remove esm no json module loading section doc: remove esm no-json-module-loading section Nov 10, 2021
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 10, 2021
@aduh95
Copy link
Contributor

aduh95 commented Nov 10, 2021

I think it's a little too early to remove this, we should probably wait for the JSON module support to stabilize first. The most reasonable option is still to either use fs.readFile + JSON.parse or module.createRequire.

@dnalborczyk
Copy link
Contributor Author

dnalborczyk commented Nov 11, 2021

I think it's a little too early to remove this, we should probably wait for the JSON module support to stabilize first. The most reasonable option is still to either use fs.readFile + JSON.parse or module.createRequire.

this is not true anymore, at least without mentioning import assertions:

JSON imports are still experimental and only supported via the `--experimental-json-modules` flag.

otherwise it implies that using this flag will allow json files to be imported by using "es6" module syntax.
everything else follows this in the paragraph as an alternative approach.

@aduh95
Copy link
Contributor

aduh95 commented Nov 11, 2021

But both are still true:

  • JSON imports are still experimental
  • they are only supported via the --experimental-json-modules flag.

I don't see how this implies it would be possible to import them without using an assertion. But more importantly, I don't understand how removing this section helps anyone at this point, we should wait for JSON imports to be no longer experimental, and supported without the --experimental-json-modules flag (assertion required or not).

Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

Marking it officially as requested change to make sure it doesn't land too soon. Before landing, we should ensure that:

  • JSON imports are no longer experimental;
  • JSON modules are no longer behind the --experimental-json-modules flag.

@GeoffreyBooth
Copy link
Member

We could perhaps edit this section to mention both the current non-experimental methods as well as the experimental method. Then later on all we’d need to do is remove the text that says import of JSON is experimental.

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM but @aduh95 is right

@GeoffreyBooth
Copy link
Member

Marking it officially as requested change to make sure it doesn't land too soon. Before landing, we should ensure that:

  • JSON imports are no longer experimental;
  • JSON modules are no longer behind the --experimental-json-modules flag.

These are both true now, and have been for a long time; and almost a year has passed. Dunno how much longer we want to wait, but it’s probably safe by now.

@aduh95
Copy link
Contributor

aduh95 commented Oct 20, 2022 via email

@GeoffreyBooth
Copy link
Member

Import assertions are going away. The proposal was replaced with import attributes. We're waiting on a V8 update to change on our side. I think it's better to keep the fs approach as the primary recommendation until this is all sorted out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. esm Issues and PRs related to the ECMAScript Modules implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants