Stage 3 Review

[bmeck]

This doesn't appear to have changed much since my last review so it is fairly simple:

https://github.com/tc39/proposal-json-modules/blob/master/spec.html#L81

if { type: 'json' } should be assert { type: 'json' }

per updated import assertions spec.

The above text implies that is recommended but not required that hosts do not use moduleRequest.[[Assertions]] as part of the module cache key. In either case, an exception thrown from an import with a given assertion list does not rule out success of another import with the same specifier but a different assertion list.

We probably don't want to imply a recommendation (since web and compatible envs itself won't do this exactly); the text above it isn't necessarily a recommendation but is an observation . Maybe a reframing:

The above text implies that is recommended but not required clarifies that hosts may or may not do not use moduleRequest.[[Assertions]] as part of the module cache key. In either case, an exception thrown from an import with a given assertion list does not rule out success of another import with the same specifier but a different assertion list.

Rest seems fine. 👍🏼

[ljharb]

It was pretty intentional that it be phrased as a recommendation - that the web can't guarantee complying with the recommendation in all cases doesn't change what's recommended.

[bmeck]

@ljharb it should not be framed that way as no env I know of would follow it.

[ljharb]

I think it needs to be framed that way even if no env follows it - but also, browsers already will be following it except where servers prevent them from doing so; i hope node does; and bundlers and transpilers certainly will.

[bmeck]

@ljharb I think it must not be framed that way. It does not match even Node's fs usage if fs manipulation occurs during the loading process. If we frame it as a recommendation we must frame it in such a way that explains that it almost certainly can never match a real env.

[bmeck]

Perhaps we could introduce our first "intention" note into the spec here? I know we had a similar issue where graph re-ordering was intended to be banned but was unable to actually be enforced.

[ljharb]

It matches a real env under normal circumstances - which includes no server shenanigans and no filesystem manipulation. It’s not a requirement because these things can’t be guaranteed not to happen, but it’s a recommendation because in their absence, the guarantees should be kept (and will be, by browsers, and hopefully by node).

[bmeck]

@ljharb I disagree, a host does not control things and they are real possibilities, not shenanigans as you so claim. If it is a recommendation to assert guarantees and the guarantees cannot be achieved it should not be a recommendation. This sounds like an intention.

[bmeck]

To be clear, this comes from the fact that we are a standards body and the goals of interoperability both through documentation and creation of behaviors is not being done with a recommendation here. Stating something about behavior outside of the intention not being considered breaking if altered seems apt, but we cannot recommend something here if it cannot be actually achieved by anyone.

[ljharb]

I’m confused; this was a requirement for me for stage 3, and i thought we’d resolved it months ago.

A host does not control these things which is why it’s not a requirement. However, in the absence of things a host does not control, the host is recommended to employ the stronger guarantee. It is very important to me that the spec language do whatever it can to maximize occurrences of the stronger guarantee (which matches what browsers will be doing in the absence of these things).

[bmeck]

@ljharb we had several calls about this in which I did state that it should not be a recommendation but was fine with a rewording.

[bmeck]

which matches what browsers will be doing in the absence of these thing

The point of our standard is to define the behaviors and stating that something is outside of behaviors we will be defining seems fine to me. Stating that a real behavior in all our implementations shouldn't happen doesn't make sense.

[ljharb]

As i recall, you stated it must not be a requirement - a recommendation is an editorial status which has no normative weight, and i was under the impression that this was acceptable. How would you suggest rewording it?

It’s a rare behavior, and stating that hosts should strive, outside a rare behavior, to behave in a stronger way, makes perfect sense to me.

[bmeck]

How would you suggest rewording it?

• Various possibilities from RFCs seems good to use, but we don't use that RFCs for wording

  • "REALLY SHOULD NOT use the assertion in the cache key" from RFC 6919 seems the best fit.
  • "MUST (BUT WE KNOW YOU WON'T)" from RFC 6919 seems possible? It is strictly stronger than a recommendation but explaining the why is complex and likely not useful to the spec text itself.
  • "MAY WISH TO use the assertion in the cache key" from RFC 6919 seems strange / implies the wrong emphasis to me as it lacks a negative association with the behavior.
  • "WOULD PROBABLY" from RFC 6919 seems strange to me.
  • "SHOULD" is what RFC 2119 uses to alias "recommended", likely it wouldn't make sense here since it states there are valid or even potentially useful behaviors (to my knowledge no useful/valid behavior is known to use this)

• "Intention" seems fine, but I would prefer we actually document somewhere what it means.

• "Clarification" seemed fine to me above but my text above doesn't state anything about intent of host implementations.

It’s a rare behavior, and stating that hosts should strive, outside a rare behavior, to behave in a stronger way, makes perfect sense to me.

Strive for and recommendation are very different.

Recommendation implies it is possible since recommending something that is not possible doesn't make sense; to my knowledge the only way this is possible is with bundling an entire codebase and pre-linking the module graph in some way to make all modules static and unable to be altered during runtime.

[ljharb]

Browsers intend to use the stronger guarantee most of the time, as would, presumably, node - that’s why i find “should” appropriate. In other words, it’s quite possible often, just not always, which is the only reason it’s not a requirement.

[bmeck]

@ljharb it isn't able to be controlled by browsers or node, how can they make a guarantee?

[ljharb]

They can’t, which is why when it happens, the weaker guarantee will kick in. Until it does happen, the stronger guarantee would hold.

[bmeck]

@ljharb the point is they can't make the stronger guarantee.

[bmeck]

@ljharb what is wrong with "REALLY SHOULD NOT"?

[ljharb]

They can make them in the common case, just not always.

“REALLY SHOULD NOT”’s definition seems like a good fit (altho “OUGHT NOT” would fit my intentions better), but I’m not sure 262 follows that RFC so far.

[bmeck]

@ljharb they cannot guarantee even the common case since it is the server which isn't an ECMAScript implementation that causes the behavior to be observed due to the cache infrastructure. Browsers cannot assert guarantees on server behaviors.

[bmeck]

@ljharb "OUGHT NOT" seems fine to me but the usage of moral implications is uncomfortable since we are not here to make moral judgements.

[dontcallmedom]

re using RFC6919 keywords, the publication date of that RFC is not a coincidence

[ljharb]

lol thanks, I’d totally missed that.

[bmeck]

I'm fine w/ whatever wording is chosen even if it is from what was originally a joke. We cannot recommend impossibilities since they cannot be done.

[ljharb]

They aren’t impossible tho, that’s the crux i think of the disagreement here. They’re impossible for all cases, which is why it’s not required, but possible in many cases, which is why it’s what’s recommended.

[bmeck]

@ljharb they are not possible for the majority of cases. Any case where there is a non-bundled form it has a race which cannot be done. If IO is considered the non-majority that would be one thing, but the vast majority of cases are subject to this. I would once again move to block if the spec is trying to state the impossible.

[ljharb]

Even if a recommendation couldn't be followed in the majority case, we could still recommend it. The spec permits handling the cases you're concerned about - why is this recommendation so problematic for you?

[bmeck]

@ljharb because you can't recommend someone do something that cannot be done. Reframing it to be recommended for when it is possible seems fine, but for the vast majority of cases that hosts are doing it isn't. I wouldn't want to state that I recommend people stop eating for example if they need to eat and then back peddle to say that they don't need to eat all the time.

[ljharb]

The current phrasing says "It is recommended but not required" which already implies "when possible", would adding the words "when possible" address your concern?

[bmeck]

Likely that would work. To my knowledge there is no implication of "when possible" in the current phrasing.

[ljharb]

Great! I just filed #11; hopefully we can land that and move forward.

[xtuc]

Just for the record, the following point has been fixed:

if { type: 'json' } should be assert { type: 'json' }

[bmeck]

LGTM now since that has merged

[xtuc]

Since we addressed the two @bmeck points, could we close this issue? and merge #12 ?

[littledan]

Wasn't @codehag another reviewer? Have we gotten signoff from her?

[codehag]

I am a reviewer. I haven't had time yet, the week since TC39 has been very busy.

[codehag]

I read it all, it looks good to me, I didn't find any outstanding issues. but I am also brain dead due to a really long week.

I can do another pass the week after next and should be able to give a definitive thumbs up. I think it is fine.

[bmeck]

@xtuc sounds good to close this and wait on the other review separately

[xtuc]

@codehag edited her message and it looks we have a 👍 too. Thanks, i'm going ahead and close the issue.