Breaking API: Allow WebsocketInitFunc to add payload to Ack (#4)

[telemenar]

• Allow WebsocketInitFunc to add payload to Ack

The connection ACK message in the protocol for both graphql-ws and graphql-transport-ws allows for a payload in the connection ack message.

We really wanted to use this to establish better telemetry in our use of websockets in graphql.

I have:

• Added tests covering the bug / feature (see testing)
• Updated any relevant documentation (see docs)

[coveralls]

coverage: 75.761% (+0.01%) from 75.748% when pulling da137ea on observeinc:master into fceb331 on 99designs:master.

[UnAfraid]

Wouldn't it be better if this change would add the functionality without breaking the compatibility?

[StevenACoffman]

@UnAfraid Do you have some recommended changes for how to accomplish this goal without breaking compatibility?

[UnAfraid]

@UnAfraid Do you have some recommended changes for how to accomplish this goal without breaking compatibility?

Sure, we can deprecate InitFunc and add new func OnInitFunc for example.
(Naming to be improved)

type InitOptions struct {
  InitPayload InitPayload
  // Room for more in the future
}

type InitDataPayload struct {
  Payload map[string]any
  // Room for more in the future
}

type OnInitFunc func(ctx context.Context, options InitOptions) (context.Context, InitDataPayload, error)

[telemenar]

I thought about going that direction but it adds potential for confusion around which api to use and there is no pressure to actually cease using the deprecated variant. So it is likely just a permanent increase of the api surface area and a small complexity increase to deal with both options.

The proposed change is compile time breaking, however it is a super fast fix to maintain existing behavior. (Add another nil to your return statements and add it to the type.)

Part of why I used a pointer type was so make this as minimal change to existing behavior as possible so it didn’t start sending an empty payload when there was previously no payload. To make sure that if you do the minimum compile fix there was 0 chance it would be behavior breaking.

[RumenNikiforovTruckstop]

In my opinion causing compile error on someone who just updated their dependencies would be more confusing, now they'll have to check why that broke, what changed, why it changed, etc..

And transport.InitPayload is already a reference type, its a map, it shouldn't be pointer

[telemenar]

Acknowledged, on the upgrade path. I personally just think that the short term hit is better than the long term complexity and confusion.

Yes the underlying map that is InitPayload is a reference type. (Pet peeve: this is only documented as part of the golang faq and not part of the golang language specification.) However, if you just look at a function using the type you don’t know that. Which is why I went with the pointer type as to me that confers optionality. I wish golang had a clear standard optional type I would have preferred to have that instead.

Also, despite my pushback, I’m perfectly happy to make changes based on the desire of the maintainers. I just want to make sure my motivations for not having taken that approach in the first place are documented.

Let me know the path I should take here.

[telemenar]

@StevenACoffman -- as maintainer preferences on the way forward here?

[StevenACoffman]

Sorry for the delay.

While it would be polite to Deprecate the old API, and give people a small (one release ~= 2 weeks) heads up that they should migrate to the new API before upgrading gives them a compile error. However, I can't think of a better name than "WebsocketInitFunc". I'm going to merge this, but I'm going to let that simmer for a bit before the next release to see if there's a large contingent of people for whom adding a default argument to this is a significant burden.

I have updated the PR title to highlight it's breaking nature.

[telemenar]

Sounds good let me know if you’d like any follow up work from me.

[dbarrosop]

Hi, I just updated my dependencies and fell into this breaking change. While I am ok with small breaking changes like this (I agree a small breaking change now is better than confusion over time) I am struggling to understand how to fix the breaking change. The PR description indicates that documentation has been fixed but I couldn't find such documentation in this PR. The function signature isn't clear either, it just says it wants me to return a pointer but nothing mentions what this pointer is for so I don't know how to fix this breaking change. Am I supposed to return nil, a pointer to the payload I got or something else? And what is this good for?

Thanks!

[dsaiko]

No any documentation, no sample provided, none of existing examples updated ...
Have no idea what I should return and I really do not want to spend my time going through the source code and guessing ...

[StevenACoffman]

@dsaiko I am sorry for your frustration, but I rely on the small coterie of regular websocket contributors. Both @UnAfraid and @telemenar have been reliable in making valuable improvements.

If this change that happened 133 days ago is now causing an unexpected problem for you, please describe what happened and how you eventually resolve it.

Please help those who follow in your footsteps, and leave the documentation better than you currently find it.

[dbarrosop]

FWIW I solved it by returning nil. To be clear, the work here is greatly appreciated, this library is really good and the websocket support just works™. However, there are a few bits here and there that are a bit underdocumented and this is one of those bits (the PR also mentions docs have been updated but the link doesn't send you anywhere useful). Given that this introduced a breaking change a quick note in how to migrate (which is as simple as adding nil) and how this could be useful would've saved some of the initial frustration (the new test is very telling and could have been reused for explaining the functionality).

Anyway, this happens in most libraries all the time so I don't think it's worth dwelling too much on it, it's the cost of using opensource so keep up with this amazing work.