Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add connection-init-payload for websocket #20

Closed
wants to merge 2 commits into from

Conversation

gabrielnau
Copy link
Contributor

Hello,

This is a follow-up of the discussion here: #19

We need to send connection_init message before any other one, that's why I propose a new handler ::connection-init that can be prepended in ::on-ws-open dispatch.

As for the changes in the tests, I had to mock ::internals/send-ws because some tests don't override it and the thus they try to send a message on a websocket that doesn't exist.

To match Apollo behavior, it is sent in a new type of message "connection_init".
Otherwise it is very easy to create a race condition between ::init and ::subscription or ::query
@gabrielnau
Copy link
Contributor Author

@oliyh I've found an issue today testing my patch where I would send messages over websocket before the connection_init message was sent. My second commit is an attempt to ensure nothing goes on the wire until the connection is acknowledged.

@oliyh
Copy link
Owner

oliyh commented Mar 22, 2018

Hi

Thanks for the PR. Am I right in thinking that your second change is unrelated to this feature? It feels like it should be a separate issue, to wait for the ack before sending any messages.

Regarding your implementation of connection-init I haven't had a chance to look at it properly yet but I hope to do so soon. I didn't think it would be necessary to add any more event handlers so I want to understand it properly.

Thanks

@gabrielnau
Copy link
Contributor Author

Hello,

Yes you're right, the use case where I need to wait for an ack is typically authentication. I can elaborate on that if you want ?

On the other hand, my understanding is that in the "protocol" (source needed), it is expected to do so. I wonder if that's a goal to match apollo's behavior or not ?

Thanks.

oliyh added a commit that referenced this pull request Mar 26, 2018
option to not have a connection init message #20
@oliyh
Copy link
Owner

oliyh commented Mar 26, 2018

Hi @gabrielnau

I have merged your first commit and made one small change such that :connection-init-payload nil will not send any message at all. Thank you for the contribution!

Your second change is a separate issue which I want to deal with separately, could you open a new PR for that?

Thanks
Oliy

@oliyh oliyh closed this Mar 26, 2018
oliyh added a commit that referenced this pull request Mar 26, 2018
@gabrielnau
Copy link
Contributor Author

Hi @oliyh,

Thanks for the review. I'm actually not sure of the proper way to handle the use case I had in re-graph. Before going forward I should have a look at apollo's implementation first and come back with clearer ideas. I hope to do that soon.

Gabriel

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants