Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Don't skip opId === 0 #179

Merged
merged 1 commit into from
Jun 12, 2017
Merged

Don't skip opId === 0 #179

merged 1 commit into from
Jun 12, 2017

Conversation

cvle
Copy link
Contributor

@cvle cvle commented Jun 12, 2017

I noticed that the first subscription is never unsubscribed.
Turns out that the first operation id is zero and that the if (opId) {
check is the culprit.

Fixes: #180

TODO:

  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

@apollo-cla
Copy link

@cvle: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@NeoPhi NeoPhi merged commit 516f47c into apollographql:master Jun 12, 2017
@NeoPhi
Copy link
Contributor

NeoPhi commented Jun 12, 2017

Great catch @cvle. Thank you for the PR!

@cvle
Copy link
Contributor Author

cvle commented Jun 12, 2017

@NeoPhi Awesome! Can you make a release? This was the source for some warnings on our side. Would be great to get rid of them :-)

@NeoPhi
Copy link
Contributor

NeoPhi commented Jun 12, 2017

@dotansimha or @Urigo See above.

@cvle
Copy link
Contributor Author

cvle commented Jun 12, 2017

@NeoPhi I saw that recently the typings of operation id was changed to a string. I didn't know. The typings should be adapted and the check should be changed to sth like: if (opId || opId === 0).

@cvle
Copy link
Contributor Author

cvle commented Jun 12, 2017

https://github.com/apollographql/graphql-subscriptions still has number typings for opIds.

@dotansimha
Copy link
Contributor

dotansimha commented Jun 13, 2017

@cvle where is the number for opIds?

@Urigo
Copy link
Contributor

Urigo commented Jun 13, 2017

thanks @cvle ! released in 0.7.3

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

Successfully merging this pull request may close these issues.

5 participants