Subscriptions support #1298

tjmehta · 2016-07-25T22:58:15Z

@josephsavona, I have manually tested simple end-to-end use-cases, and Subscriptions are working. But this is very much a WIP, and I have lots of cleanup and tests to write.

I submitted this PR to open up a channel of communication with you, as I have lots of questions. I will post my questions in comments below.

ghost · 2016-07-25T22:59:24Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

tjmehta · 2016-07-25T23:00:01Z

The first question I have is that I am unable to run npm run typecheck without errors on master (w/ none of my changes). A lot of the "errors" seem like warning complaining about the location of dependencies. For example:

^^^^^^^^^^^^^^^^^^^^ This modules resolves to "<<PROJECT_ROOT>>/../node_modules/invariant/package.json", which is outside both your root directory and all of the entries in the [include] section of your .flowconfig. You should either add this directory to the [include] section of your .flowconfig, move your .flowconfig file higher in the project directory tree, or move this package under your Flow root directory.

How do I get typecheck to pass?

EDIT:

Environment info:
os: OS X El Capitan v10.11.4
node: v4.4.6, v5.12.0, v6.2.2
npm: 3.10.5

*I ended up getting the tests/typecheck to work on ubuntu, but cannot get them to work on OSX.

ghost · 2016-07-26T00:49:05Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

edvinerikson · 2016-07-27T08:23:24Z

src/subscription/RelaySubscriptionObservable.js

+  /**
+   * Dispose underlying RelaySubscription subscription
+   */
+  unobserve(): void {


IMO this should be dispose instead.

I was debating the same (bc of rxjs terminology), but I followed the nomenclature I saw here:

relay/src/store/RelayQueryResultObservable.js

Line 110 in ec0a7f1

_unobserve(): void {

That line is for a private method, used to handle the case when a subscriber is disposed. Is this intended to be a public method that a developer would call? Or a public method that the subscription framework would call?

Aside: this PR is really big, it would be a lot easier to review as a series of incremental changes.

This method is called "publicly" by RelaySubscriptionObserver

Also, I am more than happy to break this PR down. Once you all get a macro view of this, let me know which parts would be easiest and most useful to get in first.

tjmehta · 2016-07-27T08:56:06Z

#541

tjmehta · 2016-07-28T03:56:30Z

I was able to get lint, flow, and tests passing on my ubuntu machine.
I also added a lot of tests and fixed some bugs.

Awaiting feedback. Thanks

josephsavona · 2016-07-28T07:14:44Z

@tjmehta We haven't had an opportunity to review this in detail just yet, but in the meantime I wanted to thank you for such a well thought out and thoroughly tested PR. We'll review and comment soon.

edvinerikson · 2016-07-28T07:20:03Z

src/container/__tests__/RelayContainer-test.js

+        environment,
+        mockRoute
+      );
+      expect(environment.subscribe.mock.calls[0][0]).toBe(mockSubscription);


expect(environment.subscribe).toBeCalledWith(mockSubscription);
Not super important but it simplifies a little

tjmehta · 2016-07-28T07:21:47Z

Thanks for the comment @josephsavona. Take your time and let me know. I have a few questions myself, but I will wait for feedback first.

edvinerikson · 2016-07-28T07:28:07Z

src/subscription/RelaySubscription.js

+   * are sent to the server serially and in-order to avoid unpredictable and
+   * potentially incorrect behavior.
+   */
+  getCollisionKey(): ?string {


Does this make sense with subscriptions?

You're right I didn't end up using it. Although, I had a thought to use the collision key in RelaySubscriptionObserver for de-duplication (versus instance equality - RelaySubscriptionObserver.js#44).

Ah, I see. Would probably be nice to have something that can de-duplicate subscriptions that listens on the same data. 👍

Using the same name for a different purpose seems confusing. I would suggest either a different name, or see if there is a way to dedupe subscriptions automatically (maybe from the identity of the Relay.QL object plus hash of variables?)

@josephsavona is there a util for comparing Relay.QL objects? If not, is it a deep object comparison?

kamek-pf · 2016-07-28T20:25:32Z

This is great !
@tjmehta, what did you use on the backend side ? Last time I checked subscription support in graphql-js was pretty embryonic.

tjmehta · 2016-07-28T20:28:08Z

@kamek-pf I use primus-graphql. Pushing a release soon to support subscriptions (currently working out bugs).

josephsavona · 2016-08-02T16:25:38Z

Thanks for your patience in our reviewing this. This is tricky. On the one hand, we really want to support subscriptions out of the box. On the other hand, our current strategy for Relay development is to focus on providing core primitives that developers can use to build interesting solutions in user-space (like React). We discuss this a bit more in #559:

This issue is to track work toward a new, more modular public API. We plan to strike a balance between overly monolithic and overly decomposed, and split Relay into two main parts:

Relay Core: imperative API for fetching, observing, and updating data.

React/Relay: React integration in the form of RelayContainer and RelayRenderer, implemented purely in terms of the public Relay Core API.

..and in our H1 Plans. (Aside: these aren't super visible, and we'd welcome suggestions on how to make this strategy more obvious to new contributors)

So rather than merge this into Relay, we'd be happy to support you in releasing this as a separate NPM package. Specifically, we can provide code review and API suggestions here on the initial version, and would appreciate your feedback & PRs to make sure that the public API is sufficient to build this type of project in user-space.

josephsavona · 2016-08-02T16:28:27Z

src/store/RelayEnvironment.js

@@ -28,10 +30,13 @@ const readRelayQueryData = require('readRelayQueryData');
 const relayUnstableBatchedUpdates = require('relayUnstableBatchedUpdates');
 const warning = require('warning');

+const noop = function() {};


We have a helper module for this:

const emptyFunction = require('emptyFunction');

tjmehta · 2016-08-02T17:19:38Z

@josephsavona I understand the thoughts behind this decision, and I want to understand more about your expectations of open source solutions. I am sure your team has discussed pros and cons of different potential implementations for subscriptions. This should help outline what requirements the team wants the final solution to have. I think sharing those thoughts with the OSS community would be helpful.

After experimenting with this solution for a few weeks, I definitely understand that subscriptions can be implemented w/ a variety primitives (versus observables). If the relay team decides that subscriptions should definitely use observables, the classes in this PR would help set a foundation for open source solutions to flourish (RelaySubscriptionRequest, RelaySubscriptionQuery, RelaySubscription, RelaySubscriptionObservable, ..). IMO these classes are only as opinionated as existing classes for Queries/Mutations, but I would love to hear your thoughts.

edvinerikson · 2016-08-03T06:39:53Z

@josephsavona Not really relevant to this PR but I would love if you could share the progress on the core API / prototype. It's been a while since I heard anything about it, last time was probably when you updated #559 with the new proposal.

More relevant, I have a npm package that kind of adds subscriptions into Relay, it doesn't cover all cases and are probably in need of some api changes but I think it can be a nice start to community driven subscription support. What it does is basically to provide a high-level api in terms of React components and then a low-level api in terms of classes and helper methods.
@josephsavona, I would really appreciate if someone from the Relay team are willing to take a look at it and give some feedback. edvinerikson/relay-subscriptions

@tjmehta I really like this PR, if we could get this into edvinerikson/relay-subscriptions I think that would be awesome.

tjmehta · 2016-08-03T23:25:11Z

@edvinerikson I am still going to wait for @josephsavona thoughts here. I don't believe that the subscriptions solution that I provided is overly opinionated. I believe it provides basic classes similar to Queries/Mutations, that could help the OSS community build advanced features on top of.

I read all of the threads and previous PRs before implementing this, and made sure to follow all the suggestions @josephsavona provided in: #541

tjmehta · 2016-08-04T20:09:32Z

@josephsavona, since this PR is so large, can you at least overview the classes and let me know which ones you definitely think could be in relay core (like RelaySubscriptionRequest). I can start breaking out classes into individual PRs, so that this is easier to digest. I am on vacation for a week starting tomorrow, so I will start when I get back.

josephsavona · 2016-08-04T23:55:35Z

Not really relevant to this PR but I would love if you could share the progress on the core API / prototype.

@edvinerikson Yup, we'll be publishing a blog post on this soon. For now, you can also check the meeting notes for more details on what we've been working on.

I definitely understand that subscriptions can be implemented w/ a variety primitives (versus observables) ... IMO these classes are only as opinionated as existing classes for Queries/Mutations

@tjmehta Good points and questions. First let me clarify; the primitives that I'm referring to are the low-level functions such as RelayStoreData#handleQueryPayload() to write data into the Relay store, RelayEnvironment#observe() to subscribe to the latest results of a fragment, and RelayStoreData#handleUpdatePayload() to apply the results of a mutation or subscription. They also include things like RelayQuery.Subscription and printRelayQuery that can be used to represent a subscription object and print it as GraphQL text.

You're right that the classes introduced in this PR are at a similar level of abstraction to existing classes such as RelayMutation. However, subscriptions are still an active area of investigation for the GraphQL community. There are a lot of tradeoffs to consider - should we use subscription { ... } or query @live { ... }? How to implement the backend? How about the client/server transport? We agree that real-time is important, but are hesitant to bake one solution into the core (we can't even provide a default network layer implementation).

Another consideration is that we are working on some large changes to Relay core. Accepting this PR would increase the surface area that we have to maintain and upgrade, which could slow progress for us and the community.

Given all of this, we won't be able to move forward with this PR. Instead, I think the best approach is to consider merging this into @edvinerikson's relay-subscriptions. As I've mentioned in previous comments this is high-quality, well-thought out code and we're happy to support you in releasing this as a standalone library.

tjmehta · 2016-08-05T17:39:02Z

@josephsavona Thanks for your thorough explanation. I will close this PR for now. May I use this PR as a channel of communication w/ you? Or is there a better way? The reason I ask is that I still have a lot of questions about subscriptions for you and would love to get more involved w/ relay contributions in general.

@edvinerikson, I will be in touch after I get back from vacation to see how compatible our implementations are.

josephsavona · 2016-08-05T18:01:00Z

@tjmehta Yeah, feel free to ask questions here and we'll answer as soon as we can :-)

ghost added the CLA Signed label Jul 25, 2016

edvinerikson reviewed Jul 27, 2016
View reviewed changes

tjmehta force-pushed the subscriptions branch from 9ed3b4a to de3507c Compare July 27, 2016 08:49

ghost added the CLA Signed label Jul 27, 2016

__wip: wip

06421d6

tjmehta force-pushed the subscriptions branch from de3507c to 06421d6 Compare July 28, 2016 03:49

ghost added the CLA Signed label Jul 28, 2016

edvinerikson reviewed Jul 28, 2016
View reviewed changes

ghost added the CLA Signed label Jul 28, 2016

josephsavona reviewed Aug 2, 2016
View reviewed changes

facebook-github-bot added the CLA Signed label Aug 2, 2016

ghost added CLA Signed labels Aug 3, 2016

ghost added the CLA Signed label Aug 3, 2016

ghost added the CLA Signed label Aug 4, 2016

tjmehta closed this Aug 5, 2016

This was referenced Aug 16, 2016

[meta] Support real-time updates via event subscriptions #541

Closed

Other potential API updates relay-tools/relay-subscriptions#7

Closed

edvinerikson mentioned this pull request Sep 15, 2016

Improve Container API relay-tools/relay-subscriptions#20

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscriptions support #1298

Subscriptions support #1298

tjmehta commented Jul 25, 2016 •

edited

Loading

ghost commented Jul 25, 2016

tjmehta commented Jul 25, 2016 •

edited

Loading

ghost commented Jul 26, 2016

edvinerikson Jul 27, 2016

tjmehta Jul 27, 2016

josephsavona Aug 2, 2016

tjmehta Aug 2, 2016

tjmehta commented Jul 27, 2016

tjmehta commented Jul 28, 2016

josephsavona commented Jul 28, 2016

edvinerikson Jul 28, 2016

tjmehta Jul 28, 2016

tjmehta commented Jul 28, 2016 •

edited

Loading

edvinerikson Jul 28, 2016

tjmehta Jul 28, 2016 •

edited

Loading

edvinerikson Jul 28, 2016

josephsavona Aug 2, 2016

tjmehta Aug 3, 2016

kamek-pf commented Jul 28, 2016

tjmehta commented Jul 28, 2016

josephsavona commented Aug 2, 2016 •

edited

Loading

josephsavona Aug 2, 2016

tjmehta commented Aug 2, 2016 •

edited

Loading

edvinerikson commented Aug 3, 2016

tjmehta commented Aug 3, 2016

tjmehta commented Aug 4, 2016 •

edited

Loading

josephsavona commented Aug 4, 2016

tjmehta commented Aug 5, 2016

josephsavona commented Aug 5, 2016

Subscriptions support #1298

Subscriptions support #1298

Conversation

tjmehta commented Jul 25, 2016 • edited Loading

ghost commented Jul 25, 2016

tjmehta commented Jul 25, 2016 • edited Loading

ghost commented Jul 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmehta commented Jul 27, 2016

tjmehta commented Jul 28, 2016

josephsavona commented Jul 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjmehta commented Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

tjmehta Jul 28, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamek-pf commented Jul 28, 2016

tjmehta commented Jul 28, 2016

josephsavona commented Aug 2, 2016 • edited Loading

Choose a reason for hiding this comment

tjmehta commented Aug 2, 2016 • edited Loading

edvinerikson commented Aug 3, 2016

tjmehta commented Aug 3, 2016

tjmehta commented Aug 4, 2016 • edited Loading

josephsavona commented Aug 4, 2016

tjmehta commented Aug 5, 2016

josephsavona commented Aug 5, 2016

tjmehta commented Jul 25, 2016 •

edited

Loading

tjmehta commented Jul 25, 2016 •

edited

Loading

tjmehta commented Jul 28, 2016 •

edited

Loading

tjmehta Jul 28, 2016 •

edited

Loading

josephsavona commented Aug 2, 2016 •

edited

Loading

tjmehta commented Aug 2, 2016 •

edited

Loading

tjmehta commented Aug 4, 2016 •

edited

Loading