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

Absinthe subscriptions support #70

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

karlosmid
Copy link

@churcho
Copy link

churcho commented Feb 11, 2022

🎉 👏🏿
@uesteibar please take a look at this. This would be a very good addition.
@karlosmid any chance you experimented with multiple subscriptions? Do they all have to go in the Applications children?

@uesteibar
Copy link
Owner

@churcho will do, but life has been rather crazy lately and haven't had the time yet :)

@churcho
Copy link

churcho commented Feb 14, 2022

@churcho will do, but life has been rather crazy lately and haven't had the time yet :)

Understandably so, I appreciate all the great work.

@churcho
Copy link

churcho commented Apr 8, 2022

@churcho will do, but life has been rather crazy lately and haven't had the time yet :)

Hi, any chance you can review this PR? I am trying to see if I can use this and stop relying on 3rd party apps for channel information push.

@karlosmid
Copy link
Author

@churcho you can try it for test using this:

{:neuron, git: "git@github.com/karlosmid/neuron.git"},

Copy link
Owner

@uesteibar uesteibar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! thanks for taking the time for this, sorry it took ages to review. Apart from the comment below, it'd be ace if we could add some tests to this, I don't feel confortable merging such a feature without any test examples.

You can do subscriptions:

```elixir
defmodule AddUserSubscription do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm probably missing something as to why we've chosen this API, but do you think we could make it so that the user can start it as a regular supervisor/process?

I'm not familiar with AbsintheWebSocket but as far as I can gather we can always import AbsintheWebSocket.Supervisor and build our own supervisor. wdyt?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @uesteibar AbsintheWebSocket does all the heavy lifting of WebSocket protocol implementation. This is why I decided to use this library.
AbsintheWebSocket.Supervisor does all the magic of WebSocket subscriptions. This is why it must be used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding testing. As we only add a wrapper around AbsintheWebSocket.Supervisor, AbsintheWebSocket has all the tests.
As a proof that this wrapper works, I added it in this Absinthe repo as a integration test:
https://github.com/karlosmid/zoom/tree/master

README.md has all instructions about Absinthe subscriptions.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlosmid I understand AbsintheWebSocket.Supervisor does the "magic", however I think we can still have our own supervisor by using import and thus allow folks to use them as regular processes.

Did you consider an API like the one defined in #33?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding testing, I feel strongly that if this library offers an API, we do need tests that ensure it behaves as expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @uesteibar.

Do you agree that this is what I need to do:

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good 👍 I'd also be fine if we test it without mocking AbsintheWebSocket.Supervisor, but it might be harder 😄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karlosmid did you get around to handling the 2 PR task lists?

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.

3 participants