Any reason ehy this is internalized?

nah, cut'n'paste error actually

I think it would be helpful to make sure the URL’s scheme is HTTP or HTTPS (or at least the origin is not ’null’.

There is a check for a falsy origin on line 1195.

A “null origin” is not that the returned origin is null but that it is 'null'. Try new URL('abc:').origin, which IMO should be forbidden.

ah, right, forgot about that little nuance. Sigh.

Ok, added a check for 'null' in the code and in the test

Is this TODO not currently solvable? We know for a fact that at least the origin is ASCII-only.

Yep, was already working on it :-)

Would help if what stream ID 0 (and why it must have an origin) could be explained. Not everyone likes to read RFC’s.

Maybe the !origin could be written out explicitly as origin !== undefined? (Ditto below.)

And also I think the origin.length check or 'null' check fail it should use a more informative error code.

There is also the issue of if we want to error out on invalid origin when stream is non-zero. I think we should, for the same reason as we are erroring out on non-string type regardless of what stream is. So overall I think the logic should look like

// type checks first
if (origin !== undefined && typeof origin !== 'string') error();

if (origin === undefined) {
  if (stream === 0) error();
} else {
  // we shouldn’t make exceptions for empty origin string either: either it’s undefined or it has to be a valid URL.
  origin = new URL(origin).origin;
  if (origin === 'null') error();
  if (stream !== 0) error();
}

If we simplify the signature to allow the second argument to be either a string or a number, then we don't necessarily have to jump through this hoop. Will stew on it a bit more

Hmm, would it be a good idea to make the second parameter number|string, where when it’s number it has to be a valid stream ID and when it’s a string it would be used as the origin (and thus the stream ID is implicitly 0)? That’s only a good idea if we are certain they won’t make it possible to have a stream ID and origin at the same time, but does have the advantage of hiding the 0 as an implementation detail (it’s sort of a magic number otherwise).

Thanks for writing the documentation though: it’s very helpful.

I've been stewing over it. I'm starting to have a real dislike for polymorphic function signatures but it would make things a bit easier in both the implementation and on the user.

I sort of feel this name should correspond with the name of the method on the server side. I’m ambivalent about whether alternateService is a better name than altsvc though: I feel eventually most people would refer to it as ALTSVC (HTTP/2 frame) or Alt-Svc (HTTP/1.1 header), while technically the name of the header is alternateServices.

@addaleax ... given that you were the one that made the original rename suggestion, what do you think here?

@jasnell If everybody here agrees that it’s going to be referred to as ALTSVC anyway, yea, probably best to stick with it?

Either way, yes, +1 for being consistent :)

I think altsvc is definitely going to be the most common given the way the header and frame type are defined. I'll switch the API call back :-)

Is this requirement enforced for the value string? If not then we should, to combat the HTTP header encoding debacle that only recently got rectified. In fact IMO we should verify that it only consists of HTTP tokens, similar to the HTTP header checks.

Origin is fine because it’s guaranteed to be ASCII by the URL parser.

The value is not limited to token. It will contain a sequence of uri-host constructs (host:port) within quoted strings. At the C++ level, we write these out as one-byte strings so there is at least some level of enforcement there. And on the receiving side, these are treated as one byte strings so the value will be received correctly. Anyone using extended characters will just see those get corrupted. Because of the performance implications and the fact that this is a non-critical API, I don't think we need to validate every character.... although, it would be good to extend the documentation on this a bit more -- as you said, not everyone likes to read RFCs :-)

The additional documentation is always a plus.

Anyone using extended characters will just see those get corrupted.

Well that’s certainly not the best of outcomes. If this is truly a non-critical API, I think we should ensure correctness above performance. Good call on quotation marks though, and in that case I think simply using a /[\x00-\x7f]/ check should be enough.

There may also be some security implications involved, when '\uff20' is treated identically to `'\u0020' by the V8 API.

Ok, added a check

Is the x32 meant to be x20?

Also, x80-xff can’t really happen because the specific quoted-string is specified to contain (optionally) a url-host, a colon, and a port number, all of which is in the ASCII range. Removing support for that will make this regex not generic for quoted-string but I think it’s a good change.

oh ha!! thinking hex, typed decimal. sigh

I'd still prefer to keep the x80-xff range given the definition of quoted-string. Yes, it shouldn't happen, but the spec definition technically allows for it. It will be up to the user to validate that what they get is a valid value.

Okay. I can’t agree with that interpretation of the spec but this is not a blocker for me like the corruption issue was.

Maybe this could mention the clear special value as well.

Yeah, good idea

The length check is unnecessary: serialized origins cannot be empty.

it is not clear where this number can be found.

I think we would be better of in accepting a URL object rather than a string, or an object with an origin  property. The typical usage is to send this down for every session, and passing an object allows a user to avoid parsing a URL for every request.

Let's support both. If originOrStream is passed in as an object with an origin property, it will be used. This risks, of course, the value not being strictly ASCII, but we can document that requirement.