Adds Support for Graphql ^0.11.0 #261

DxCx · 2017-08-30T06:28:02Z

This PR adds support for GraphQL ^0.11.0
^0.10.0 will still work.

BREAKING CHANGE: Remove support for SubscriptionManager
Related to:
apollographql/graphql-subscriptions#104

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

Closes #259

edvinerikson · 2017-09-07T18:15:13Z

What's the status of this? No one able to review it? @DxCx

DxCx · 2017-09-07T18:26:37Z

@edvinerikson there is still issue with graphql-subscriptions which apollo team needs to decided either remove from dependancies or upgrade as well..
also, as you can see there are older PRs which havn't been reviewed as well.. im waiting as well..

edvinerikson · 2017-09-07T18:32:24Z

Alright, I see. Do you know what the dependency is inside of graphql-subscriptions?

I pinged @stubailo on Slack with the link to my PR and mentioned the low activity. I have not get any reply yet though (20min ago or something).

I really need this change because of the async subscribe function.

DxCx · 2017-09-07T18:35:29Z

only subscription manager interface which i think needs to be just moved inside this package until deprecated or just be deprecated as soon as possible.
but there is no reason of graphql-subscriptions to be a dependancy here.

edvinerikson · 2017-09-07T18:39:50Z

I agree. I just migrated away from it when I upgraded. Not sure in which version it got deprecated but making a major bump and removing it shouldn't be too hard(?).

stubailo · 2017-09-07T18:55:45Z

I think @NeoPhi will also take a look soon, let's try to get some next steps before the end of the week.

zackify · 2017-09-08T01:09:56Z

@stubailo anything I can do to help get this out quicker? I was stuck for a long time today until finding this haha

NeoPhi · 2017-09-08T02:13:54Z

I'm in favor of merging this in and having the needed changes to graphql-subscriptions made. There has been a lot of confusion about the deprecation warning so adding 0.11 support as part of a version bump on this project and the other one feels like the right next steps.

edvinerikson · 2017-09-08T05:25:52Z

Something I would like to have fixed too is the error handling. Not in this PR though. I spent quite a few hours trying to debug why it didn't work and it turns out the library catches its own errors and sends straight to the client without any way of interception on the sever. E.g. Only thing I had to go on was "cannot read property .then of undefined" without stack trace. I didn't even know if it was a server or client error. ¯\_(ツ)_/¯ What's your thoughts about this?

…

On Fri, 8 Sep 2017 at 04:13, Daniel Rinehart ***@***.***> wrote: I'm in favor of merging this in and having the needed changes to graphql-subscriptions made. There has been a lot of confusion about the deprecation warning so adding 0.11 support as part of a version bump on this project and the other one feels like the right next steps. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADtdjKcv-86mpezRWjQBioWXxspqIxRbks5sgKLlgaJpZM4PG7Cw> .

DxCx · 2017-09-08T07:36:53Z

@NeoPhi whats the reason for wanting to keep this as a dependnacy?
It was called deprecated ~4 month ago...

NeoPhi · 2017-09-08T13:24:26Z

@DxCx Sorry if my comment was unclear. I'm not advocating that graphql-subscriptions be kept as a dependency. Instead I'm saying that if we remove the dependency here we should also update graphql-subscriptions to remove the deprecated SubscriptionManager to avoid any confusion.

DxCx · 2017-09-08T15:33:20Z

Oh, that is absolutely correct. :)

BREAKING CHANGE

DxCx · 2017-09-12T10:44:45Z

will need a new version of graphql-subscriptions so CI won't fail on node 4
but accept for that it looks ready for review to me.

NeoPhi · 2017-09-12T19:11:30Z

@DxCx graphql-subscriptions v0.5.0 has been released.

dotansimha · 2017-09-13T05:02:11Z

package.json

    "@types/lodash": "^4.14.68",
    "@types/mocha": "^2.2.41",
    "@types/node": "^8.0.8",
    "@types/sinon": "^2.3.0",
    "@types/ws": "^3.0.0",
    "chai": "^4.0.2",
-    "graphql": "^0.10.1",
+    "graphql": "^0.11.3",
+    "graphql-subscriptions": "^0.4.4",


@DxCx update to latest :)

yeah i was working at that since there was a typing still depended.. now it's pushed =)

dotansimha · 2017-09-13T05:02:29Z

src/adapters/subscription-manager.ts

@@ -1,110 +0,0 @@
-import { SubscriptionManager } from 'graphql-subscriptions';


🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉

dotansimha · 2017-09-13T05:04:37Z

@DxCx looks great. just need to update graphql-subscriptions version and fix CI issue.
Back then we talked about "V1" of the transport, and the goal was to remove the support for the old code.
If we removed the SubscriptionManager support, do you think we need to remove the support for the older protocol as well? @DxCx @Urigo

DxCx · 2017-09-13T05:34:18Z

we don't need to remove it but i think we can remove all deprecated code..
we can do it now or next step will be to clean them up and release graphql-transport-ws

NeoPhi · 2017-09-13T11:49:36Z

As this is a breaking change release, I'd be in favor of removing the deprecated code.
I think removal of the old protocol should be part of the switch to graphql-transport-ws as @mistic has thoughts on that.

DxCx · 2017-09-13T12:06:51Z

so do we want to remove all the legacy API and keep the protocol legacy layer for the time being?
(until graphql-transport-ws)

NeoPhi · 2017-09-13T12:15:46Z

That would be my recommendation.

DxCx · 2017-09-13T12:21:53Z

@stubailo @Urigo @mistic @dotansimha @martijnwalraven would be great to get some advice in here..

stubailo · 2017-09-13T18:00:03Z

I agree with both of you! Let's go ahead and do it.

NeoPhi · 2017-09-13T18:44:40Z

@DxCx Do you have a preference on next steps? My thinking is that the changes to remove the deprecated API are simple and mirror what is happening with the removal of SubscriptionManager. We can then do a breaking change release of subscriptions-transport-ws that gets this package working with 0.11.x. Then as part of the switch to the new package name the old protocol can be removed.

DxCx · 2017-09-13T19:19:58Z

Yeah lets merge this and wait with releasing ill open another PR cleaning all deprecated api keeping the legacy protocol support

…

On Wed, 13 Sep 2017 at 21:44 Daniel Rinehart ***@***.***> wrote: @DxCx <https://github.com/dxcx> Do you have a preference on next steps? My thinking is that the changes to remove the deprecated API are simple and mirror what is happening with the removal of SubscriptionManager. We can then do a breaking change release of subscriptions-transport-ws that gets this package working with 0.11.x. Then as part of the switch to the new package name the old protocol can be removed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#261 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABB8W4nJcQbYY4QqNw1ui7Tu4IHNEZ7Kks5siCKagaJpZM4PG7Cw> .

mistic · 2017-09-14T15:20:05Z

@DxCx @NeoPhi I agree with you! In my opinion we should release now a breaking change version without the SubscriptionsManager and the legacy protocol support. And that should be the last subscriptions-transport-ws release. Next we should release a new package called graphql-transport-ws. What do u think about this?

stubailo · 2017-09-14T15:24:05Z

Hmm, why not release the breaking change as the new package right away?

NeoPhi · 2017-09-14T16:00:18Z

I think releasing the new package with Apollo 2 support would make it easier for people to understanding the breaking protocol changes.

mistic · 2017-09-14T17:12:24Z

Both ways are valid. However I think that releasing the breaking changes (of things already marked as deprecated for 4 months) in the subscriptions-transport-ws as it very last released version can help people to instant migrate to the new package with 0 effort. What do u think @stubailo @NeoPhi @DxCx @dotansimha @Urigo ?

DxCx · 2017-09-14T17:12:30Z

@NeoPhi
this package is already supported by apollo-link-ws and works with apollo-client 2 🎉

@mistic
IMO we should release the cleaned api version (Other PR merged)
and right after rename package and release again with just rename,
then from there removing the legacy protocol support and total cleanup.

after that we can continue "grow" the new graphql-transport-ws package.

mistic · 2017-09-14T17:45:02Z

@DxCx wasn't this the same I suggest? :D

In order to allow everyone understand and no doubts remaining what we are suggesting is:

Release the 0.9.0 version of subscriptions-transport-ws with the cleaned api, the legacy protocol and without the SubscriptionsManager
Rename the repository (so we can keep the stars and all the other history) and all the documentation for the new graphql-transport-ws.
Clean the old protocol support, update again the documentation and release the first 0.0.1 version of the graphql-transport-ws deprecating the old npm subscriptions-transport-ws package with a message to use the new one instead.

What do u think @DxCx @NeoPhi @dotansimha ?

DxCx · 2017-09-14T17:46:37Z

Agreed.
I just think initial version can be 0.1.0

mistic · 2017-09-14T17:53:22Z

yeah @DxCx it can be 👍

As soon as the others write their thoughts I can proceed with the agreed changes 😄

NeoPhi · 2017-09-14T18:13:27Z

@mistic Sounds good. Let me know if you'd like me to handle any of that.

morajabi · 2017-09-18T19:42:10Z

Whatever are you doing, please add some notes or guides to upgrade... I wasted a few hours because there's a lot of conflict between documentation of Apollo and this package. Thank you for the great work here.

DxCx mentioned this pull request Sep 7, 2017

Make it compatible with new GraphQL.js version #268

Closed

4 tasks

DxCx added 4 commits September 12, 2017 13:14

chore(package): initial support for graphql 0.11.1

3c76419

chore(package): add graphql as peer dependancy

fdd95f7

chore(package): update CHANGELOG.md

0d0512a

chore(package): deprecated SubscriptionManager

637df74

BREAKING CHANGE

DxCx force-pushed the gql-0.11.1 branch from 679f24b to 637df74 Compare September 12, 2017 10:14

DxCx changed the title ~~Adds Support for Graphql ^0.11.0 [DO NOT MERGE]~~ Adds Support for Graphql ^0.11.0 Sep 12, 2017

doc(package): update README.md

dcd4aa5

dotansimha reviewed Sep 13, 2017

View reviewed changes

chore(package): update graphql-subscriptions to v0.5.0

7efc0ab

dotansimha approved these changes Sep 13, 2017

View reviewed changes

chore: cleanup old tests

02fee59

NeoPhi merged commit 71dde80 into apollographql:master Sep 13, 2017

DxCx deleted the gql-0.11.1 branch September 14, 2017 06:44

DxCx mentioned this pull request Sep 14, 2017

[BREAKING CHANGE]: Cleanup for deprecated API #272

Merged

4 tasks

nickofthyme mentioned this pull request Oct 1, 2017

Update Readme for v0.9.0 #284

Closed

jalalat mentioned this pull request Nov 18, 2017

[Modern] Subscriptions and live queries docs and examples facebook/relay#1655

Closed

ianmartorell mentioned this pull request Jan 17, 2018

subscribeToMore: GraphQL subscription error (cannot read property 'then' of undefined) #277

Closed

		@@ -1,110 +0,0 @@
		import { SubscriptionManager } from 'graphql-subscriptions';

Adds Support for Graphql ^0.11.0 #261

Adds Support for Graphql ^0.11.0 #261

Conversation

DxCx commented Aug 30, 2017 • edited Loading

edvinerikson commented Sep 7, 2017

DxCx commented Sep 7, 2017

edvinerikson commented Sep 7, 2017

DxCx commented Sep 7, 2017

edvinerikson commented Sep 7, 2017

stubailo commented Sep 7, 2017

zackify commented Sep 8, 2017

NeoPhi commented Sep 8, 2017

edvinerikson commented Sep 8, 2017 via email

DxCx commented Sep 8, 2017

NeoPhi commented Sep 8, 2017

DxCx commented Sep 8, 2017

DxCx commented Sep 12, 2017

NeoPhi commented Sep 12, 2017

dotansimha Sep 13, 2017

Choose a reason for hiding this comment

DxCx Sep 13, 2017

Choose a reason for hiding this comment

dotansimha Sep 13, 2017

Choose a reason for hiding this comment

dotansimha commented Sep 13, 2017

DxCx commented Sep 13, 2017

NeoPhi commented Sep 13, 2017

DxCx commented Sep 13, 2017 • edited Loading

NeoPhi commented Sep 13, 2017

DxCx commented Sep 13, 2017

stubailo commented Sep 13, 2017

NeoPhi commented Sep 13, 2017

DxCx commented Sep 13, 2017 via email

mistic commented Sep 14, 2017

stubailo commented Sep 14, 2017

NeoPhi commented Sep 14, 2017

mistic commented Sep 14, 2017

DxCx commented Sep 14, 2017 • edited Loading

mistic commented Sep 14, 2017

DxCx commented Sep 14, 2017

mistic commented Sep 14, 2017

NeoPhi commented Sep 14, 2017

morajabi commented Sep 18, 2017

DxCx commented Aug 30, 2017 •

edited

Loading

DxCx commented Sep 13, 2017 •

edited

Loading

DxCx commented Sep 14, 2017 •

edited

Loading