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

esm: decouple json modules from cjs cache #30592

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Nov 22, 2019

I've opened this to discuss implementation issues with JSON modules on web and/or module attributes that could arise from the coupling as briefly mentioned in the Modules WG meeting. This is not meant to land while discussion (in particular about module attributes) continues.

CC: @nodejs/modules

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@bmeck bmeck added discuss Issues opened for discussions and feedbacks. experimental Issues and PRs related to experimental features. esm Issues and PRs related to the ECMAScript Modules implementation. labels Nov 22, 2019
@ljharb
Copy link
Member

ljharb commented Nov 22, 2019

It seems like this change would mean that mutating a required JSON would no longer be visible from an imported JSON, and vice versa. Could you elaborate on why this change is a good thing?

@bmeck
Copy link
Member Author

bmeck commented Nov 22, 2019

@ljharb this more closely matches how web is looking at introducing JSON modules in a variety of ways. In particular the coupling seems problematic to assumption of module attributes being able to assert that loading JSON does not introduce side effects. In addition, JSON modules on the web get unique cache entries per URL, the current implementation by using the CJS loading mechanism ignores URL query and fragments.

I'm not sure preserving mutating JSON as a feature has been weighed against web compatibility concerns and this is a PR that could encourage that discussion.

I personally think that mutating JSON isn't the highest priority feature, and the leveraging of cache busting via URL leads to odd behavior coordination for users as seen in nodejs/modules#417 . For CJS, that seems fine as there isn't another runtime interoperability being proposed, however for JSON there is some interoperability discussion to be had.

@guybedford
Copy link
Contributor

Do we have an issue / agenda item for this to discuss further? It would help to understand the full motivations again here.

@bmeck
Copy link
Member Author

bmeck commented Nov 26, 2019

@guybedford right now the topics are kind of spread out, I think discussing it here might be a place to coordinate. Mostly this deals with compatibility and expectations and the few links above for now. I could probably tie it to other issues, but nothing really seems to be pressing except those 2.

  • Things like cache busting not working for JSON as seen in import(cjs) with query strings has odd behavior modules#417 seems solvable while I think for CJS it would be problematic due to __filename and __dirname (we could add a __url ? :shudder: ).
  • As for compatibility it seems that JSON would be keyed off URL including search and fragment in the web so that should be discussed.
  • As for module attributes, it seems by round tripping through CJS we are in a situation that somewhat invalidates the idea of the JSON parser being isolated that it assumes and that came up in the Modules WG meeting last time. We can expand upon if that seems ok here or if it proves problematic for that proposal for us to implement a different loading for JSON.

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Dec 24, 2019
@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
@bmeck
Copy link
Member Author

bmeck commented Apr 12, 2021

closing as this looks to be deferred to JSON modules which are landing in the spec

@bmeck bmeck closed this Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants