Explictly indicate WebSockets version

[RubenVerborgh]

The current WebSockets protocol is in draft, and will likely go through multiple versions in the future. This PR proposes to explicitly indicate the version, both from the client and the server side.

This change aims to be backwards compatible with existing client implementations.

[acoburn]

I am very supportive of this change.

One minor comment about the name of the subprotocol solid-ws-draft/v0.1.0-alpha: if you look at the current IANA registry of WebSocket subprotocols, none of them use a slash in their names. I don't know that a slash is incorrect (and I don't want this to become a long bikeshedding conversation), but it doesn't seem entirely conventional. OTOH, the version is clearly alpha, and so in that sense, anything would be fine.

[RubenVerborgh]

if you look at the current IANA registry of WebSocket subprotocols, none of them use a slash in their names

Interesting list, thanks.
I see that the naming convention is (unfortunately) quite random. I had imitated the / from the User-Agent header (but then I should probably drop the v, upon closer inspection). No strong feeling; could also be other character like @ (npm style), or - or .. I meanly see . in the list.

[csarven]

As the identifier is used in context of websockets protocol, I think solid/0.1.0-alpha would suffice. I'd interpret that along the lines of "HTTP/0.9".

GitHub malfunction. Will submit a new review.

[csarven]

As the identifier is used in context of websockets protocol, I think solid/0.1.0-alpha would suffice. I'd interpret that along the lines of "HTTP/0.9". Only the version bit (after the slash) needs to be updated going forward. If registered with IANA, it would simplify to stick to digits for the version so that the value can be reused.

[kjetilk]

Looks good, I think that the explicit mention of alpha makes it sufficiently clear it is a draft that is versioned.

[pmcb55]

LGTM

[melvincarvalho]

-1 I would like to block this merge while I speak to @timbl and @johnbizguy

My basic objection is along the lines of substantive, and potentially breaking changes, being made should incur a change to the version number of the spec. Whether that is a minor version can be discussed

I would suggest a major version as it can then be a bridge to the future spec. There is a whole other repo for that future work

The MIT work up to 0.7 should be frozen as it has major work and implementations. It has not been, and that is already problematic, and causing production problems

New changes with a new team and new process should be based on a new version number

[RubenVerborgh]

There will be a version increase after changes like this one indeed. Does that solve your problem?

[csarven]

-1 I would like to block this merge while I speak to @timbl and @johnbizguy

Like over a telephone call?

I agree with you on this repo being frozen and ongoing work is happening elsewhere. Perhaps we can clarify the former. FWIW, the intention of this PR was to temporarily mitigate existing risks in implementing Solid's WebSocket. Think of it as a hot patch or a worthwhile errata to bring forward with good intentions. As mentioned, it is backwards compatible, so anyone that happens to run into this will be aware.

Note that if WebSocket is adopted in the ongoing solid specification, it can implement or at the very least consider this. See also solid/specification#163 .

[melvincarvalho]

A version change in connection with this change would be acceptable, yes

[RubenVerborgh]

That will happen, of course. Can you unblock this issue then if nothing else?

Unblocked as requested contingent on bumping the spec version

[johnbizguy]

Let me know what I can do to help. Thanks, John

…

On Apr 8, 2020, at 5:52 AM, Sarven Capadisli ***@***.***> wrote:  -1 I would like to block this merge while I speak to @timbl and @johnbizguy Like over a telephone call? I agree with you on this repo being frozen and ongoing work is happening elsewhere. Perhaps we can clarify the former. FWIW, the intention of this PR was to temporarily mitigate existing risks in implementing Solid's WebSocket. Think of it as a hot patch or a worthwhile errata to bring forward with good intentions. As mentioned, it is backwards compatible, so anyone that happens to run into this will be aware. Note that if WebSocket is adopted in the ongoing solid specification, it can implement or at the very least consider this. See also solid/specification#163 . — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[RubenVerborgh]

Given the approvals, I will merge. It's marked as needing a version bump.

[melvincarvalho]

@johnbizguy thanks for the response

The basic request is that the specification version in the README is incremented, corresponding to this change

Given the general agreement on this, I've removed the blocking objection

However, It has not been done yet

So the request is to bump the version, as a next step, and just ensure it doesn't get kicked into the long grass

[RubenVerborgh]

Yeah the problem is that there are some other changes pending. We shouldn't bump for one minor change. Should either merge all soon and bump, or ensure that another branch is the visible one.

[johnbizguy]

We discussed as an exec team this morning and keen to do what we can to help. It looked as though Melvin and Ruben aligned on what’s appropriate? Thanks, John

…

On Apr 8, 2020, at 2:06 PM, Ruben Verborgh ***@***.***> wrote:  Yeah the problem is that there are some other changes pending. We shouldn't bump for one minor change. Should either merge all soon and bump, or ensure that another branch is the visible one. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[melvincarvalho]

@johnbizguy thanks much for the offer of assistance!

Unfortunately, we are not quite in sync, yet

My objection was a simple one, and that is, that was that this merge, should be accompanied with a version bump

Unfortunately, that hasnt happened yet, and there seems to now be a shift of the goal posts. See the comment "We shouldn't bump for one minor change"

What I suggest is to either ahead and bump the version as agreed, or revert the change, and think again

[johnbizguy]

Seeing what we can do to help

[RubenVerborgh]

Hi @melvincarvalho, thanks for your patience.

Here's what I've done for you to address this issue:

1. Added a tag to the frozen 0.7.0, which will never change: https://github.com/solid/solid-spec/tree/v0.7.0
2. Linked to that frozen 0.7.0, as it is the currently active version: bf299eb
3. Bumped the version to indicate that the WebSockets adjustment is part of the next version (and thus not of 0.7.0): cb1373a

We do not consider 0.7.1 finished yet, so are reluctant to publish it as such. Hence the choice for v.0.7.0-next and the explicit mention of it being evolving.

However, should this not satisfy your concerns, please let me know at your earliest convenience and we will release the current v.0.7.0-next as v0.7.1 immediately.

Thank you for your help and input.

[melvincarvalho]

Not looked in detail, but I think I can live with this. Thanks for the quick response.

[melvincarvalho]

Looked at this closer. I can live with this in principle. One slight detail:

The v0.7.0 tag should match the commit c9a8214 (or earlier). I believe the commit after that was the first that came about with a new work flow

This is actually nearer to the actual release date of 0.7.0 and matches the CHANGELOG

That segments the MIT created spec from a newer process, newer team, newer spec. Although actually there is a whole other repository for the newer spec items

This is also quite close to the very good suggestion made my @michielbdejong in #199

My basic request is to have a clean separation. There is mainly upside, and very little downside, in that it's largely just a label, and good house keeping

If we can agree on that, I think we're good

[RubenVerborgh]

Changed v0.7.0 to point to c9a8214 per direct.

[melvincarvalho]

Thanks. That works for me

[melvincarvalho]

@johnbizguy @timbl thanks again for you attention, I think this is a useful issue for your exec team to look at, even if casually

In particular, consider the following statements:

Sarven: Think of it as a hot patch or a worthwhile errata

and

Ruben: We shouldn't bump for one minor change

And looking at the actual change:

The client _SHOULD_ include the protocol version `solid/0.1.0-alpha`
in the `Sec-WebSocket-Protocol` header.

Implication 1: solid client apps and solid client libraries (e.g. rdflib.js) are compliant with the previous spec, but not this one. Client tests will fail.

if the set of values obtained
from parsing the `Sec-WebSocket-Protocol` header
does not contain `solid/0.1.0-alpha`,
then the server SHOULD emit a warning
and SHOULD close the connection:

Implication 2: of all solid servers compliant with the previous version of the spec, none will be compliant with this version. Server tests will fail.

Implication 3: none of the solid realtime apps will work against a server that complies with this spec. Apps will run against old servers, but not against new. Backward compatible, it is not, IMHO

If you are minded to, I would invite your exec team, to consider the implications to solid of this change. And come to a conclusion as whether they think the change constitutes a minor version bump, a major version bump, or none at all. Perhaps just as a useful thought experiment, if nothing else.

Just writing this out for completeness. As I say, I can live with the current resolution

[RubenVerborgh]

@melvincarvalho Respectfully, but the above is technically incorrect. Please do not cause unnecessary worries about compatibility breakage, because there is none.

Given that existing clients do not send the header, the part you copied above does not apply. This invalidates implications 1 and 3.

The warning is a SHOULD. This invalidates implication 2.

Can we now please focus our energy on things that have impact? We’ve spent too much time on this already.

[csarven]

I find Ruben's response to the implications that Melvin raises is satisfactory within this PR.

@melvincarvalho As we are operating under the W3C Solid Community Group and the Solid process, I think we agree that the issue you've raised about versioning have been acknowledged and followed up with updates which you've approved. If you'd like to continue the implications in a manner beyond this PR or a new issue, the best place to take it up would be in a solid/specification meeting (in chat or weekly call), possibly in solid/process or solid/chat if you prefer. To the best of my knowledge, there is no other "official" grouping that can adequately and transparently take this up.

[RubenVerborgh]

So given that @melvincarvalho's reaction is limited to a 👎, I will lock this issue now to prevent any further unnecessary draining of resources, which has already bitten us multiple times in the past. The issue has been dealt with. I will also hide the comment with technically inaccurate information. Anything else needs a new issue, and likely not in this repository.

If you want to pursue this, I will explicitly request valid evidence of either a) a real technical problem; or b) any other member of the community considering this a problem.

Following @csarven, I also want to emphasize that the connection made to a company is irrelevant here. In particular, my statements are my own. This issue and repository belong to the Solid community; all else is a misunderstanding on which we should not spend any more time.

[melvincarvalho]

I do not wish my response to be censored. Therefore, I have unhidden. Would appreciate if it was left that way.

[RubenVerborgh]

Sorry, the not spreading of incorrect information is more important. You deliberately altered parts of spec text in order to make false claims about incompatibility.

Please stop interaction with this issue now; it has been locked for that reason.