esm: add experimental .json support to loader #43

MylesBorins · 2019-02-26T17:49:58Z

With the new flag --experimental-json-modules it is now possible
to import .json files. It piggy backs on the current cjs loader
implementation, so it only exports a default. This is a bit of a
hack, and it should potentially have it's own loader, especially
if we change the cjs loader at all.

The behavior for .json in the cjs loader matches the current
planned behavior if json modules were to be standardized, specifically
that a .json module only exports a default.

Refs: nodejs/modules#255
Refs: whatwg/html#4315
Refs: WICG/webcomponents#770
Refs: whatwg/html#4407

MylesBorins · 2019-02-26T17:52:28Z

Plan to flesh this out with better documentation if this gets support. This is also more or less the way we can introduce a flagged CJS approach. Although tbh after playing with IRP I think we may need to do a pretty big change to the loader if we drop importing cjs...

devsnek · 2019-02-26T17:57:52Z

@MylesBorins would you mind adding separate loader logic for this? i would worry about things like adding named exports to cjs having bad interactions with this. you can copy from from https://github.com/devsnek/esm_loader/blob/7aa7acc13b338630707301d0ea55dc56ec446e30/src/index.js#L80-L87 or https://github.com/nodejs/node/blob/ccd6b12fec944c0e77fc60ffd8cea9781d850a2e/lib/internal/modules/esm/translators.js#L113-L127

MylesBorins · 2019-02-26T19:11:15Z

@devsnek done PTAL

lib/internal/modules/esm/translators.js

jkrems · 2019-02-26T19:14:13Z

I assume the fact that it doesn't share state with require'd JSON is a feature? E.g. mutating one will not mutate the other.

test/es-module/test-esm-json.mjs

ljharb

Seems like it should share the require cache, and wouldn’t have the same issues as sharing between cjs and esm would?

MylesBorins · 2019-02-26T21:49:56Z

@ljharb I'm unsure if it sure be modifiable at all. I think "how it should be cached" or "if prestine copy is created for each import" is a hanging question

ljharb · 2019-02-26T21:51:49Z

I very much doubt it will be immutable or recreated for each import (the spec likely prohibits that anyways); if it is mutable, it seems strange not to put the object in require.cache at execution time if not already present.

jdalton · 2019-02-26T21:53:09Z

I assume the fact that it doesn't share state with require'd JSON is a feature? E.g. mutating one will not mutate the other.

I donno. Importing CJS via ESM as in the current experimental modules approach does-not-dup cache entries. I'd expect a similar thing with JSON.

I'm leaning 👎 for this PR because of cache duplication. However, if immutability was introduced it'd be 👎 for sure. IMO at that point CJS is the preferred way to consume JSON modules.

Update:

Clarified stance.

MylesBorins · 2019-02-26T22:01:20Z

@jdalton WHATWG is already starting work on this proposal. Why are you -1?

MylesBorins · 2019-02-26T22:02:46Z

@jdalton please also take a look at #41 which would completely remove json and .node from the esm loader entirely

ljharb · 2019-02-26T22:19:58Z

@MylesBorins i was under the impression that whatwg folks insisted on normal mutability, and not a fresh copy every time.

jdalton · 2019-02-26T22:23:50Z

@MylesBorins

WHATWG is already starting work on this proposal. Why are you -1?

I'm aware of the proposal. I referenced the WHATWG JSON proposal as one of the reasons I was okay having CJS default-export-only enabled behind a flag (from last week's out of band Module WG meeting) 🙃. While for the record I'd like named exports for all-the-things, my main gripe with the PR as-is is cache duplication. Secondary gripe would be immutability if it was introduced (not a fan).

devsnek · 2019-02-26T22:39:41Z

@ljharb this has normal mutability uses the esm cache

ljharb · 2019-02-26T22:51:00Z

@devsnek sure - i think "mutable and not recreated for each import" is a necessity. I'm just not clear on why a JSON module couldn't effectively be require.cache[pathToThisFile] = obj; export default obj;.

MylesBorins · 2019-02-26T23:02:50Z

yeah seems like there was a misunderstanding in my statement. 100% it should be mutable. I think the question is if it is a shared / cached singleton.

My impression from the conversations in other spaces is that each import would be a fresh copy. Happy to iterate in whatever the direction the group feels makes sense... also can take discussion to other spaces for advice on approach

MylesBorins · 2019-02-26T23:46:01Z

From the WHATWG issue it seems like caching is expected. Would people like to see the module cached in the CJS cache and then reflected into the ESM cache?

ljharb · 2019-02-26T23:58:45Z

I would prefer them to be the same; if they differ, it seems like it would be a very strange interop story.

thw0rted · 2019-02-27T07:21:56Z

Is there an issue where mutability is being discussed? I saw the whatwg discussion about it but nobody seems to describe use cases either for or against mutability -- just a bunch of assertions that one design or the other is "better" without saying why.

lib/internal/modules/esm/translators.js

test/es-module/test-esm-json-cache.mjs

MylesBorins · 2019-03-07T06:13:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/21291/
Windows run with path substitution stuff disabled: https://ci.nodejs.org/job/node-test-commit-windows-fanned/25254/

Refs: nodejs/modules#180 PR-URL: #6 PR-URL: #12 Co-authored-by: Myles Borins <MylesBorins@google.com> Co-authored-by: John-David Dalton <john.david.dalton@gmail.com>

Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal PR-URL: #28

This reverts commit 1b0695b.

There are currently two supported values "explicit" and "node"

With the new flag `--experimental-json-modules` it is now possible to import .json files. It piggy backs on the current cjs loader implementation, so it only exports a default. This is a bit of a hack, and it should potentially have it's own loader, especially if we change the cjs loader at all. The behavior for .json in the cjs loader matches the current planned behavior if json modules were to be standardized, specifically that a .json module only exports a default. Refs: nodejs/modules#255 Refs: whatwg/html#4315 Refs: WICG/webcomponents#770

MylesBorins · 2019-03-11T22:59:39Z

CI: https://ci.nodejs.org/job/node-test-pull-request/21443/

MylesBorins · 2019-03-13T20:46:41Z

landed in 3cfe571

@sapegin

BUGFIXES * [`27cccfbda`](npm/cli@27cccfb) [#223](npm/cli#223) vulns → vulnerabilities in npm audit output ([@sapegin](https://github.com/sapegin)) * [`d5e865eb7`](npm/cli@d5e865e) [#222](npm/cli#222) [#226](npm/cli#226) install, doctor: don't crash if registry unset ([@dmitrydvorkin](https://github.com/dmitrydvorkin), [@isaacs](https://github.com/isaacs)) * [`5b3890226`](npm/cli@5b38902) [#227](npm/cli#227) [npm.community#9167](https://npm.community/t/npm-err-cb-never-called-permission-denied/9167/5) Handle unhandledRejections, tell user what to do when encountering an `EACCES` error in the cache. ([@isaacs](https://github.com/isaacs)) DEPENDENCIES * [`77516df6e`](npm/cli@77516df) `licensee@7.0.3` ([@isaacs](https://github.com/isaacs)) * [`ceb993590`](npm/cli@ceb9935) `query-string@6.8.2` ([@isaacs](https://github.com/isaacs)) * [`4050b9189`](npm/cli@4050b91) `hosted-git-info@2.8.2` * [#46](npm/hosted-git-info#46) [#43](npm/hosted-git-info#43) [#47](npm/hosted-git-info#47) [#44](npm/hosted-git-info#44) Add support for GitLab subgroups ([@mterrel](https://github.com/mterrel), [@isaacs](https://github.com/isaacs), [@ybiquitous](https://github.com/ybiquitous)) * [`3b1d629`](npm/hosted-git-info@3b1d629) [#48](npm/hosted-git-info#48) fix http protocol using sshurl by default ([@fengmk2](https://github.com/fengmk2)) * [`5d4a8d7`](npm/hosted-git-info@5d4a8d7) ignore noCommittish on tarball url generation ([@isaacs](https://github.com/isaacs)) * [`1692435`](npm/hosted-git-info@1692435) use gist tarball url that works for anonymous gists ([@isaacs](https://github.com/isaacs)) * [`d5cf830`](npm/hosted-git-info@d5cf830) Do not allow invalid gist urls ([@isaacs](https://github.com/isaacs)) * [`e518222`](npm/hosted-git-info@e518222) Use LRU cache to prevent unbounded memory consumption ([@iarna](https://github.com/iarna)) PR-URL: nodejs/node#29023 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

MylesBorins requested a review from guybedford February 26, 2019 17:49

MylesBorins force-pushed the experimental-json branch from cca6cc2 to 955de99 Compare February 26, 2019 17:51

This was referenced Feb 26, 2019

JSON Modules nodejs/modules#255

Closed

JSON modules whatwg/html#4315

Closed

JSON "modules" WICG/webcomponents#770

Closed

bmeck reviewed Feb 26, 2019

View reviewed changes

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved

devsnek reviewed Feb 26, 2019

View reviewed changes

test/es-module/test-esm-json.mjs Show resolved Hide resolved

devsnek approved these changes Feb 26, 2019

View reviewed changes

jkrems approved these changes Feb 26, 2019

View reviewed changes

MylesBorins added the modules-agenda label Feb 26, 2019

ljharb approved these changes Feb 26, 2019

View reviewed changes

MylesBorins mentioned this pull request Feb 26, 2019

Node.js Foundation Modules Team Meeting 2019-02-27 nodejs/modules#276

Closed

nodejs-ci force-pushed the modules-lkgr branch from a4955e2 to 7c92011 Compare February 27, 2019 09:07

jdalton reviewed Mar 6, 2019

View reviewed changes

lib/internal/modules/esm/translators.js Show resolved Hide resolved

jdalton reviewed Mar 6, 2019

View reviewed changes

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved

jdalton reviewed Mar 6, 2019

View reviewed changes

lib/internal/modules/esm/translators.js Outdated Show resolved Hide resolved

MylesBorins force-pushed the experimental-json branch from d178e75 to 609975d Compare March 6, 2019 16:54

guybedford reviewed Mar 6, 2019

View reviewed changes

test/es-module/test-esm-json-cache.mjs Show resolved Hide resolved

MylesBorins force-pushed the experimental-json branch from 337063e to 75deb18 Compare March 7, 2019 06:10

MylesBorins added the modules-agenda label Mar 7, 2019

MylesBorins force-pushed the experimental-json branch from b0765f7 to 694df4e Compare March 7, 2019 07:07

nodejs-ci force-pushed the modules-lkgr branch from bd5c877 to 0237465 Compare March 7, 2019 09:06

guybedford approved these changes Mar 7, 2019

View reviewed changes

jdalton approved these changes Mar 7, 2019

View reviewed changes

MylesBorins force-pushed the experimental-json branch 2 times, most recently from 74a3185 to 2233250 Compare March 8, 2019 02:43

guybedford and others added 5 commits March 11, 2019 15:43

esm: minimal kernel

f4a8199

Refs: nodejs/modules#180 PR-URL: #6 PR-URL: #12 Co-authored-by: Myles Borins <MylesBorins@google.com> Co-authored-by: John-David Dalton <john.david.dalton@gmail.com>

esm: irp type implementation

d95c33d

Refs: https://github.com/GeoffreyBooth/node-import-file-specifier-resolution-proposal PR-URL: #28

esm: Revert "esm: Remove --loader."

84593be

This reverts commit 1b0695b.

doc: esm flags, package scope and file extensions rules

97bfb31

esm: add --es-module-specifier-resolution

ea59221

There are currently two supported values "explicit" and "node"

MylesBorins force-pushed the modules-lkgr branch from bec588f to ea59221 Compare March 11, 2019 22:56

MylesBorins force-pushed the experimental-json branch from 2233250 to a30dfda Compare March 11, 2019 22:58

nodejs-ci force-pushed the modules-lkgr branch from ea59221 to c6d1b01 Compare March 12, 2019 08:06

MylesBorins mentioned this pull request Mar 13, 2019

Node.js Foundation Modules Team Meeting 2019-03-13 nodejs/modules#292

Closed

nodejs-ci force-pushed the modules-lkgr branch from c6d1b01 to b4abd3a Compare March 13, 2019 08:06

MylesBorins closed this Mar 13, 2019

ljharb deleted the experimental-json branch March 13, 2019 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: add experimental .json support to loader #43

esm: add experimental .json support to loader #43

MylesBorins commented Feb 26, 2019 •

edited

Loading

MylesBorins commented Feb 26, 2019

devsnek commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

jkrems commented Feb 26, 2019

ljharb left a comment

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

jdalton commented Feb 26, 2019 •

edited

Loading

MylesBorins commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

jdalton commented Feb 26, 2019 •

edited

Loading

devsnek commented Feb 26, 2019 •

edited

Loading

ljharb commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

thw0rted commented Feb 27, 2019

MylesBorins commented Mar 7, 2019

MylesBorins commented Mar 11, 2019

MylesBorins commented Mar 13, 2019

esm: add experimental .json support to loader #43

esm: add experimental .json support to loader #43

Conversation

MylesBorins commented Feb 26, 2019 • edited Loading

MylesBorins commented Feb 26, 2019

devsnek commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

jkrems commented Feb 26, 2019

ljharb left a comment

Choose a reason for hiding this comment

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

jdalton commented Feb 26, 2019 • edited Loading

MylesBorins commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

jdalton commented Feb 26, 2019 • edited Loading

devsnek commented Feb 26, 2019 • edited Loading

ljharb commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

MylesBorins commented Feb 26, 2019

ljharb commented Feb 26, 2019

thw0rted commented Feb 27, 2019

MylesBorins commented Mar 7, 2019

MylesBorins commented Mar 11, 2019

MylesBorins commented Mar 13, 2019

MylesBorins commented Feb 26, 2019 •

edited

Loading

jdalton commented Feb 26, 2019 •

edited

Loading

jdalton commented Feb 26, 2019 •

edited

Loading

devsnek commented Feb 26, 2019 •

edited

Loading