Replace `?sandbox: text` with `?sandbox: text / null` in the spec?

[thiagowfx]

Follow-up-of: #292

Could you let me know if the following is an error in the spec?

The spec says:

1. Let |sandbox| be the value of the "sandbox" field in |command parameters|, if present, or null otherwise.

.i.e. null is possible. However the type specification is:

?sandbox: text

Should it have been

?sandbox: text / null

...instead? Other places in the spec have / null to denote explicit nullability possible.

The alternative would be to eliminate the explicit or null otherwise language. Here I am specifically thinking of undefined vs null in TS/JS.

I am happy to send a PR if you confirm such a fix is indeed desirable.

[thiagowfx]

cc @whimboo @jgraham @sadym-chromium

[jgraham]

The idea is that if you specify a sandbox you always have to provide a name. But internally we use null to represent the case where it wasn't specified, which mean "no sandbox".

We could of course accept null as a value here equivalent to not specifying the sandbox property at all. But I think any change should come in the context of a general discussion about whether we always require the missing value default to be equivalent to a specific valid value.

[thiagowfx]

I am fine with either choice, the point is that these two parts of the spec are contradicting each other at the moment.

It seems your original intent was to have null as an explicit possible value.

In this case, should we update it to ?sandbox: text / null to make it consistent with the first item in the description?

(We could of course discuss it further e.g. in the next monthly sync if you think it's warranted.)

[jgraham]

I don't think there's a contradiction; there's two different variables with different types.

In pseudo typescript code it's something like

type PreloadScriptEntry = [string, string | null];

class Session {
  preloadScriptMap: Map<string, PreloadScriptEntry>;
}

class AddPreloadScriptParameters {
   expression: string;
   sandbox?: string;
}

class AddPreloadScriptResult {
  script: string;

  constructor(script: string) {
    this.script = script;
  }
}

function addPreloadScript(session: Session, parameters: AddPreloadScriptParameters): AddPreloadScriptResult {
    let expression = parameters.expression;
    let sandbox: text | null = parameters.sandbox ? parameters.sandbox : null;
    let script = uuid();
    session.preloadScriptMap[script] = [expression, sandbox];
    return new AddPreloadScriptResult(script)
}

I get that in typescript this looks kind of odd; we're using undefined in one place and null in the other. But spec-wise it makes sense; undefined isn't usually used as a spec type, so we just directly check if the field is present, and if not use null for the internal representation, without accepting null as an explicit value in the JSON (where undefined also isn't a value). So the spec as-is requires that you either provide a string sandbox or you omit the field. We don't allow specifying null as a value, and the fact that the prose uses null to represent the missing value is totally editorial and doesn't really reflect any implementation constraint.

[thiagowfx]

and the fact that the prose uses null to represent the missing value is totally editorial and doesn't really reflect any implementation constraint.

This is the key part. Then this is WAI.

As someone who is getting ramped up in the spec for the first time, I'd say:

Let |sandbox| be the value of the "sandbox" field in |command parameters|, if present, or undefined otherwise.

would cause less confusion than the current form:

Let |sandbox| be the value of the "sandbox" field in |command parameters|, if present, or null otherwise.

And there's precedent for that, e.g.

Let type be the value of the type field of local protocol value or undefined if no such a field.

That said, if it's purely editorial I'll keep it in mind and let the type definition have priority over the prose whenever there's a situation of ambiguity like this.

Thanks for the detailed explanation, and feel free to close this bug (maybe I'd keep it open for a while just in case Maksim or Henrik want to chime in.)

[jgraham]

I think we should probably fix this by removing "undefined" from the specification, except where we literally mean the ECMAScript value undefined. In general there's some cleanup we could do around our use of spec types (e.g. we're representing protocol values as the infra map type, so instead of taking about a field being present, we should write something like if |command parameters| contains "<code>sandbox</code>"). But that kind of editorial cleanup hasn't been a very high priority.