-
Notifications
You must be signed in to change notification settings - Fork 21
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
Should JSON modules be frozen? #3
Comments
I see some emoji-reacts but what I really want is comments describing your opinions and rationale! |
Ageed. What are use-cases that are benefitted by having JSON modules not frozen? |
The same use cases as currently requiring mutable json in node - using it as a shared bit of mockable data. |
@ljharb You can mock with import maps. |
@ljharb Do you have any references for practical cases where this comes up in Node.js? It'd be helpful to understand concrete instances, if possible. |
I think it'd be pretty natural to want to mutate the returned data in some way, but whatwg/html#4315 (comment) makes me cautious. Maybe if we do deep-freeze, we provide a named export to get a fresh, mutable clone of the data? import data, { makeMutable } from "./foo.json";
assert(Object.isFrozen(data));
assert(typeof makeMutable === 'function');
assert(typeof makeMutable() === 'object');
assert(!Object.isFrozen(makeMutable());
assert(makeMutable() !== makeMutable()); |
@michaelficarra import maps aren't in the language, so they aren't a viable solution. @littledan nothing concrete; there's tons of code that Either way, I'd be more in favor of every |
I feel the same. Producing a deeply frozen has 3 major issues IMO:
|
If it is mutable we cannot produce a new object at each import location. Mutability aside, you really don't want more than one copy of a json module, since modules can't be easily gc'd. |
I'm drawn towards mutability because it seems most consist with what you would get if the default export of a module were an object. Even if you cannot modify the exports of a module namespace, you can modify the exported objects themselves. I don't see why it should be any different here just because the default export was synthetically generated by parsing JSON. |
It seems like the imported JSON is per-Realm. If I change it in this Realm, that Realm will get the original version of it. I think a JSON module return object with utils (makeMutable) in the above comment is a bit strange to me. |
@devsnek Why would we produce a new object at each import location? I guess that's technically permitted by the spec (from each different referrer), but HTML would not do this, as it just uses the referrer to produce an absolute URL, which is the key in the module map. I think we should permit HTML to process JSON modules in a similar way. @jridgewell Remember to use @Jack-Works The whole module map would be per-Realm in HTML and environments similar to it, as formalized in the Realm/HTML integration proposal. However, we don't have anything in these specs that requires separate Realms to have separate module maps. I'm not sure whether we should change that. But I think formalizing/requiring invariants that are already true in several JS environments is a bit of an orthogonal project to this proposal. |
@littledan I don't want a new object at each location, I want the same mutable object everywhere (per realm) |
@devsnek Good, that's what the current proposal is. Can you say why you want this? |
Maybe (If I already need to write |
Mutability is the default in JS, you have to ask to Object.freeze something. Also, I'd expect most people to not write type json, but to let their transpiler/bundler add it in, since deno or the browser are the only places you can't know "it's json" from the filesystem. |
I don't directly care whether it is mutable or not, but I do care that it is the same object everywhere, and I care about memory usage. It is already not ideal to import json, as I said above, and I don't want to compound that with having to copy the structure to modify it. |
@devsnek Good thing no one in this thread has proposed that it'd be a separate object on each import by default. I agree that that would be very weird. |
@littledan i suggested that in https://github.com/tc39/proposal-module-attributes/issues/54#issuecomment-625946943 - and I’d find it less weird than making it frozen. |
Of course; I’m saying that I’d prefer the bad outcome of “always fresh” over the worse outcome of “frozen”. I think it should just be a realm-wide mutable object. |
OK, given this discussion, I'm going to leave the proposal as a Realm-wide mutable object for now. Let's keep this open to discuss alternatives; we can also talk about it at the next TC39 meeting where module attributes are discussed. I'd like to consider the frozen vs not-frozen question to be something that's an important detail to figure out before Stage 3, but that we can go to Stage 2 just based on the "core" decisions (namely, that JSON modules can be imported by |
@Jack-Works recently and @robpalme previously suggested that JSON modules be frozen. We previously concluded that they would not be frozen.
However, in the context of JSON modules potentially standardized by TC39, maybe we could reconsider, and freeze JSON modules this time. I would be OK either way, personally.
The PR for JSON modules includes them as un-frozen. This wouldn't necessary prohibit a host/implementation from going around and freezing it (depending on the metaphysics of spec reading), but it'd be nice to really have a solid, cross-environment decision one way or another.
I want to argue that this is something that we can iterate on during Stage 2 and need to decide on before Stage 3.
The text was updated successfully, but these errors were encountered: