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

feat: subscription installer #266

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

Conversation

barnabasJ
Copy link
Contributor

Contributor checklist

  • Bug fixes include regression tests
  • Features include unit/acceptance tests

@@ -183,57 +184,158 @@ if Code.ensure_loaded?(Igniter) do
end
end

def setup_application(igniter) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created these two functions because it felt like there was a pattern. Looking at it afterwards, this all feels pretty phoenix specific with Pheonix.Socket and Endpoints.

I would refactor this and make these defps that are called as part of setup_phoenix. WDYT?

@barnabasJ
Copy link
Contributor Author

As subscriptions are opt in at the moment, we should also make it a flag for the installer to add it?

@barnabasJ
Copy link
Contributor Author

@zachdaniel could you give this an quick glance please and let me know if I should continue this way?

@zachdaniel
Copy link
Contributor

Do you think subscriptions could just be unflagged now? I feel like no one uses it because it's behind a flag 😆. Lets just open it up and install this automatically.

@zachdaniel
Copy link
Contributor

Some tests would be good to add, specifically for the complex case of adding the parser

@barnabasJ
Copy link
Contributor Author

Some tests would be good to add, specifically for the complex case of adding the parser

The parser part is actually not new 😅 but I agree. I'll look into adding some installer tests in general.

@barnabasJ
Copy link
Contributor Author

Do you think subscriptions could just be unflagged now? I feel like no one uses it because it's behind a flag 😆. Lets just open it up and install this automatically.

@zachdaniel I feel pretty good about it after the experiment I made last week. Only thing to consider is that we should maybe rename the pubsub option in the subscription dsl. Because the way Absinthe subscriptions are called you actually need to configure the Endpoint here and not the PubSub directly?

At least it tripped me up, when I implemented subscriptions last week.

Not sure I'm happy with just calling it enpoint either though?

@zachdaniel
Copy link
Contributor

I think that module is anything with a broadcast function right?

@barnabasJ
Copy link
Contributor Author

I think that module is anything with a broadcast function right?

It's something that implements the Absinthe.Subscription.Pubsub so i guess pubsub is probably the right word. It just tripped me up because I tried to configure the Phoenix.PubSub and that didn't work.

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