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

Feature/subscription support #173

Merged

Conversation

coladarci
Copy link
Contributor

@coladarci coladarci commented Sep 28, 2018

#4

  • Remove init chain
  • Add subscription support
  • Update readme
  • Add tests

Allow clients to submit subscription requests and get, in response, a EmberApolloSubscription which the client can use in a few ways:

  • subscription.on('event', data => do_something(data)) <-- act on this event in some way
  • subscription.lastEvent <-- this will be the most recently received event payload

@coladarci coladarci force-pushed the feature/subscription-support branch 3 times, most recently from d119860 to 600c9ab Compare September 28, 2018 23:29
@coladarci
Copy link
Contributor Author

@bgentry would love your thoughts on a few things, in particular here:

  1. See the thread in the issue around why I switched away from init() in you query manager. This makes sense to you?

  2. As I was writing the documentation, I realized that it's silly that I tacked this onto the QUERY manager (since these are subscriptions). That said, forcing two mixins is also silly, so perhaps renaming (egh) the mixin to be more general is something to consider?

  3. general ergonomics of this subscription pattern as far as how I've exposed the event data.

Thanks!

@cdeeter
Copy link

cdeeter commented Nov 1, 2018

Hey! This PR solves the problem I'm having with this library. Is there any timeline on when this will be looked at, or should I use a fork? @bgentry

@tchak
Copy link

tchak commented Nov 2, 2018

Hey @coladarci, thank you for the work on this!

I tried this PR and I am running in to the issue that apparently ApolloClient.subscribe takes a query option and not a subscription option:
https://www.apollographql.com/docs/react/api/apollo-client.html#ApolloClient.subscribe

Am I missing something?

@coladarci
Copy link
Contributor Author

coladarci commented Nov 2, 2018

Thanks for taking a look! Eek - my Readme was wrong!

My "real life" example looks like this:

      subscription: this.get('apollo').subscribe({
        query    : subscription,
        variables: {
          id: model.get('id'),
        },
      }, 'update'),

I will update the docs....

Appears the tests were also doing it but because of the heavy stubbing it wasn't causing any problems. I will update those as well!

@ynnoj
Copy link

ynnoj commented Jan 31, 2019

What's the status of this PR? Would love to see subscriptions land soon! 🤞

@coladarci
Copy link
Contributor Author

@ynnoj biggest help would be probably to point to my fork, follow the readme and see if there’s anything that be improved.. if it works out of the box and you’re thrilled, it’ll probably help push this through.

@ekristen
Copy link

ekristen commented Feb 2, 2019

I'm trying to use your example but I'm getting an error about markSubscriptionsStale not being a function.

@coladarci
Copy link
Contributor Author

@ekristen thanks for giving it a try - can you provide a little more? Stacktrace? A little more about your implementation?

@ekristen
Copy link

ekristen commented Feb 2, 2019

Ember is still pretty new to me, I tried implementing as little as possible. I have it working sans subscriptions.

I just tried copying what was in the readme into a route for the most part.

export default Route.extend(RouteQueryManager, {
  apollo: service(),

  setupSubscription() {
    this.get("apollo")
        .subscribe({ query: subscription }, "resource")
        .on("event", event => alert(`${event} was just born!`));
  },

  model() {
    const repos = this.get('apollo').watchQuery({ query }, "repositories");
    return repos;
  },
});
Error while processing route: repositories this.get(...).markSubscriptionsStale is not a function beforeModel@http://localhost:4200/assets/vendor.js:68978:26
superWrapper@http://localhost:4200/assets/vendor.js:53392:23
applyHook@http://localhost:4200/assets/vendor.js:57310:16
runSharedModelHook@http://localhost:4200/assets/vendor.js:57763:20
runBeforeModelHook@http://localhost:4200/assets/vendor.js:57737:14
tryCatcher@http://localhost:4200/assets/vendor.js:59328:14
invokeCallback@http://localhost:4200/assets/vendor.js:59500:15
publish@http://localhost:4200/assets/vendor.js:59486:9
@http://localhost:4200/assets/vendor.js:51795:16
invoke@http://localhost:4200/assets/vendor.js:27482:17
flush@http://localhost:4200/assets/vendor.js:27402:25
flush@http://localhost:4200/assets/vendor.js:27561:25
_end@http://localhost:4200/assets/vendor.js:28007:26
end@http://localhost:4200/assets/vendor.js:27749:13
_run@http://localhost:4200/assets/vendor.js:28052:21
_join@http://localhost:4200/assets/vendor.js:28028:24
join@http://localhost:4200/assets/vendor.js:27803:20
join@http://localhost:4200/assets/vendor.js:16439:12
handleEvent/<@http://localhost:4200/assets/vendor.js:55858:19
flaggedInstrument@http://localhost:4200/assets/vendor.js:13780:20
handleEvent@http://localhost:4200/assets/vendor.js:55857:17
handleEvent@http://localhost:4200/assets/vendor.js:54910:14
setupHandler/<@http://localhost:4200/assets/vendor.js:55216:22
dispatch@http://localhost:4200/assets/vendor.js:5523:16
add/elemData.handle@http://localhost:4200/assets/vendor.js:5332:6
router.js:929

@coladarci
Copy link
Contributor Author

coladarci commented Feb 2, 2019

Can you try removing apollo: service(), ?

For more context, what this library does is a little confusing to be honest - the base query manager MIXIN that you add to your route adds the following for you:

export default Mixin.create({
  apolloService: service('apollo'),
  apollo: computed('apolloService', function() {
    return this.get('apolloService').createQueryManager();
  }),
});

Notice how apolloService is "the service called apollo" and apollo is actually an instead of the queryManager NOT the service.

SO, you going in and overriding apollo to be what you'd expect it to be causes problems.. It should be defined for you.

@ekristen
Copy link

ekristen commented Feb 2, 2019

Removed the service, I'm really new to ember, if there's a full simple working example that would be best. I just started looking at the docs and what examples I could find to piece things together. I don't get the error anymore, but I don't see any subscriptions happening. I'll dig a bit more and see what I am doing wrong.

@coladarci
Copy link
Contributor Author

My assumption is you never called the setup function you defined above.. can add this.setupSubscription() into your model hook?

@ekristen
Copy link

ekristen commented Feb 2, 2019

This?

export default Route.extend(RouteQueryManager, {
  setupSubscription() {
    this.get("apollo")
        .subscribe({ query: subscription }, "resource")
        .on("event", event => console.log(`${event} was just born!`));
  },

  model() {
    this.setupSubscription();
    const repos = this.get('apollo').watchQuery({ query }, "repositories");
    return repos;
  },
});

@coladarci
Copy link
Contributor Author

Yup provided you imported that subscription you are using at the top of your file

@ekristen
Copy link

ekristen commented Feb 2, 2019

I did, but I get an error.

Error while processing route: repositories this.get(...).subscribe(...).on is not a function setupSubscription@http://localhost:4200/assets/ui.js:3779:83
model@http://localhost:4200/assets/ui.js:3783:7
deserialize@http://localhost:4200/assets/vendor.js:45615:14
applyHook@http://localhost:4200/assets/vendor.js:57312:16
runSharedModelHook@http://localhost:4200/assets/vendor.js:57763:20
getModel@http://localhost:4200/assets/vendor.js:57988:14
tryCatcher@http://localhost:4200/assets/vendor.js:59328:14
invokeCallback@http://localhost:4200/assets/vendor.js:59500:15
publish@http://localhost:4200/assets/vendor.js:59486:9
@http://localhost:4200/assets/vendor.js:51795:16
invoke@http://localhost:4200/assets/vendor.js:27482:17
flush@http://localhost:4200/assets/vendor.js:27402:25
flush@http://localhost:4200/assets/vendor.js:27561:25
_end@http://localhost:4200/assets/vendor.js:28007:26
end@http://localhost:4200/assets/vendor.js:27749:13
_run@http://localhost:4200/assets/vendor.js:28052:21
_join@http://localhost:4200/assets/vendor.js:28028:24
join@http://localhost:4200/assets/vendor.js:27803:20
join@http://localhost:4200/assets/vendor.js:16439:12
exports.bind/<@http://localhost:4200/assets/vendor.js:16540:14
mightThrow@http://localhost:4200/assets/vendor.js:3875:21
resolve/</process<@http://localhost:4200/assets/vendor.js:3943:12```

@coladarci
Copy link
Contributor Author

OK - I think I know what's going on, and I think it's an issue w/ the docs - thanks for your patience.. I've updated the docs accordingly.

The issue is:

this.get("apollo").subscribe() returns a promise, which has to resolve before you can call .on on it.

While ember provides a ton of ways to do that, the simplest is to probably invoke the subscribe in a model hook, and then in a later hook (I'd suggest setupController) you can setup your .on handler, if desired. ie

model() {
  return this.get("apollo")
             .subscribe({ query }, "human");
},

setupController(controller, model) {
  model.on("event", event => alert(`${event.name} was just born!`));
},

If you want your model to return multiple things, ember helps w/ that:

model() {
   return Ember.RSVP.hash({
     one: onePromise(),
     two: twoPromise(),
   });
},

setupController(controller, model) {
  model.one; // the result of onePromise
  model.two; // the result of twoPromise
},

@ekristen
Copy link

ekristen commented Feb 2, 2019

Is it always on event? I have everything working rendering now, but I'm not getting any events, so just trying to troubleshoot.

@coladarci
Copy link
Contributor Author

Assuming your server is publishing them - did you do all the channel stuff in the readme to setup your websockets?

Copy link
Member

@bgentry bgentry left a comment

Choose a reason for hiding this comment

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

Looks great overall! 👏

Main issues are some syntax/styling things to stay consistent. Other than that, if folks can confirm this is working well, I'd love to get this merged and released ASAP!

README.md Outdated Show resolved Hide resolved
README.md Outdated

`my-app/gql/subscription/new-human`

```
Copy link
Member

Choose a reason for hiding this comment

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

can you add gql after the triple backticks so this gets syntax highlighting?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
tests/unit/mixins/component-query-manager-test.js Outdated Show resolved Hide resolved
tests/unit/mixins/object-query-manager-test.js Outdated Show resolved Hide resolved
tests/unit/mixins/route-query-manager-test.js Outdated Show resolved Hide resolved
@ekristen
Copy link

ekristen commented Feb 8, 2019

@coladarci I got further but now event is being reported as [object Object]

@coladarci
Copy link
Contributor Author

@ekristen can you clarify what you mean by "reported"? can you paste the code that is reading out the event?

@ekristen
Copy link

ekristen commented Feb 8, 2019

nevermind, I found the problem.

@ekristen
Copy link

ekristen commented Feb 8, 2019

nevermind I didn't but I'm giving up, it's taking too much time to figure out why this lib isn't working. @bgentry did you get it working?

@coladarci
Copy link
Contributor Author

@ekristen I am very interested in understanding why you aren't getting what you are expecting. This is working quite well for us and it'd be a shame to not get you over the finish line given how much time we've spent

@ekristen
Copy link

ekristen commented Feb 9, 2019

I'm determined to make this work. QQ, is there a way to use this outside a model? Like application controller to listen to a specific subscription all the time?

@coladarci
Copy link
Contributor Author

I'm assuming you mean "Outside the model hook of a route?". In which case, yes, absolutely. The reason I suggested the model hook was so that the promise would resolve before continuing to render the route. There are a bunch of ways to pull this off w/out using the model hook.

As far as specifically putting this in a Controller, I'd reco against that for a bunch of reasons (I'm happy to go into that more if you want to provide some more reasoning for it being there in the first place). If your goal is to have it "listen all the time" then your best bet would be to use the ApplicationRoute.

Does this help?

@ChristianEllerbrock
Copy link

ChristianEllerbrock commented Mar 8, 2019

Any plans on merging this PR soon?

I have tested subscriptions inside a service and it works like a charm. However, I have noticed some issues with the documentation:

  1. To be consistent with the other examples, the example subscription file should be
    app/gql/subscriptions/new-human (and not my-app/gql/subscription/new-human)

  2. The example subscription file also is missing its ".graphql" extension. Without extension it will not be found. It should be
    app/gql/subscriptions/new-human.graphql

  3. The code-example "Subscribing from inside a route" uses double quotes instead of the common single quotes.

Btw, we are using a backend with apollo server. It maybe would seem more natural to have such an example within the section "Enabling Websockets" than the provided "Phoenix + Absinthe" example.

@coladarci
Copy link
Contributor Author

Thank you @ChristianEllerbrock ! So glad to hear it worked for you. I've updated the docs as per your feedback. Elixir has sufficiently trained me to never use single quotes :)

As far as your btw - I didn't follow that piece. I'm happy to make more docs changes I just don't quite understand your ask!

@bgentry
Copy link
Member

bgentry commented Mar 9, 2019

I was partly waiting for a 2nd user to confirm that this was working well for them, thanks @ChristianEllerbrock !

Merging now, will release once CI completes. Thanks @coladarci ! ✌️

@bgentry bgentry merged commit 42cd030 into ember-graphql:master Mar 9, 2019
josemarluedke pushed a commit to josemarluedke/ember-apollo-client that referenced this pull request Mar 9, 2019
Built-in support for subscriptions.
@josemarluedke josemarluedke mentioned this pull request Mar 9, 2019
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.

None yet

7 participants