UrqlUseSubscription by kiraarghy · Pull Request #60 · teamwalnut/rescript-urql

kiraarghy · 2019-06-05T10:28:33Z

Created UrqlUseSubscription.

Schmavery · 2019-06-05T19:30:42Z

src/components/UrqlSubscription.re

+      ~children: subscriptionRenderProps('a) => React.element,
+    ) =>
+  switch (variables, handler) {
+  | (Some(v), Some(h)) =>


(Sorry for violating the instructions in the title, but in the off-chance it saves you some time, I think you can pass through an optional named argument in jsx with <SubscriptionJs variables=?v />)

Thank you so much, so much simpler! 😄

parkerziegler · 2019-06-09T02:15:29Z

@kiraarghy I pulled locally and I think the issue had to do with webpack getting all mixed up with which copies of which dependencies to use. Specifically, we're in a weird spot in our repo setup because we yarn link to the src directory to pull reason-urql in. However, this means our examples get all of the linked dependencies' dependencies 😞(i.e. from local node_modules and from node_modules/reason-urql/node_modules/* brought in by yarn link). To resolve this, we use webpack's module resolution to tell it where to look for dependencies. That way, we guarantee that it only ever uses a single copy of a dependency, like urql or react, at runtime.

The commit I pushed up also moves us more down the path of [@bs.deriving abstract]. Overall things are looking ace and this is pretty darn close. I'm itching to get it merged ASAP so we can release a v1 before I go on PTO on Wednesday 😱But we still have to circle back for at least cursory docs so that may be a little too soon for release.

kiraarghy · 2019-06-10T09:56:54Z

Okay I think this is ready for review now!

gugahoa

Hello, all!
This PR would introduce some breaking change into the library, I pointed out in the comments where it happens

Another detail: The re-export of UrqlUseSubscription is missing from ReasonUrql.Hooks
It would also be really cool if there was an example of the useSubscription hook

gugahoa · 2019-06-10T15:46:21Z

examples/5-subscription/src/app/Messages.re

-};
+[@react.component]
+let make = () =>
+  <Subscription query variables=None handler=None>


This is a breaking change, variables and handler were optional args, now they're required and with type optional
Read more at UrqlSubscription.re about this

After the changes, this should work as before:

Suggested change

<Subscription query variables=None handler=None>

<Subscription query>

gugahoa · 2019-06-10T15:48:04Z

src/components/UrqlSubscription.re

+  external make:
+    (
+      ~query: string,
+      ~variables: option(Js.Json.t),


When you declare a named argument with type option('a), it does not mean the argument is optional to the caller, it means it receives a option('a) type.
I believe what you wanted was: ~variables: Js.Json.t=?

Suggested change

~variables: option(Js.Json.t),

~variables: Js.Json.t=?,

gugahoa · 2019-06-10T15:48:21Z

src/components/UrqlSubscription.re

+    (
+      ~query: string,
+      ~variables: option(Js.Json.t),
+      ~handler: option(handler('a, 'b)),


The same as above:

Suggested change

~handler: option(handler('a, 'b)),

~handler: handler('a, 'b)=?,

src/components/UrqlSubscription.re

gugahoa · 2019-06-10T15:49:17Z

src/components/UrqlSubscription.re

+      ~handler: option(handler('a, 'b)),
+      ~children: subscriptionRenderProps('a) => React.element,
+    ) =>
+  <SubscriptionJs query ?variables ?handler>


Suggested change

<SubscriptionJs query ?variables ?handler>

<SubscriptionJs query variables handler>

@gugahoa with your changes in place, i.e. variables as ~variables: Js.Json.t=? and ~handler: handler('a, 'b)=? I believe this can stay as is now cc/ @Schmavery.

kiraarghy · 2019-06-11T16:16:33Z

Hey I'm probably not going to be able to have a look at this till tomorrow evening, given @parkerziegler's desire to get the next release I'm happy for someone else to finish it off?

parkerziegler · 2019-06-11T16:31:35Z

@kiraarghy I won't have time for release since I'm full time on client work and can only work on this stuff in the evening. I'll try to carry it through sometime tomorrow morning so we can have a v1 release ready to go for next week. We may make a second beta release as well to test these things before cutting a v1.

1. Created UrqlUseSubscription.

1. Added rei for useSubscription. 1. Updated UqlSubscription.re to use new syntax. 1. Updated bs-platform to 5.0.4

1. Started on examples.

… and useSubscription bindings.

Didn't mean to commit :S

Co-Authored-By: Gustavo Aguiar <gugahoa@gmail.com>

parkerziegler · 2019-06-19T05:08:25Z

src/UrqlTypes.re

+};
+
+/* The signature of the structure created by calling `.make()` on a `graphql_ppx` module. */
+type request('response) = {


We'll want to use this type in useQuery and useMutation cc/ @gugahoa @Schmavery.

parkerziegler · 2019-06-19T05:15:47Z

src/components/UrqlSubscription.re

-[@bs.module "urql"]
-external subscriptionComponent: ReasonReact.reactClass = "Subscription";
-
+[@bs.deriving abstract]


There are some follow up changes to make here, but I'm going to make a separate issue to track making the components type safe in the same manner as the hooks.

parkerziegler · 2019-06-19T05:33:32Z

src/hooks/UrqlUseSubscriptionWithHandler.re

+  let response =
+    switch (fetching, data, error) {
+    | (true, None, _) => UrqlTypes.Fetching
+    | (true, Some(d), _) => Data(d)


So urql has a strange case where fetching is never set to false on subscriptions (I've pinged folks internally to ensure that's intended behavior). To handle this, I needed to add this extra case to this pattern matching to check if we are receiving data even as fetching remains true.

parkerziegler · 2019-06-19T05:39:54Z

src/hooks/UrqlUseSubscriptionWithHandler.re

+      handler: UrqlTypes.parsedHandler('acc, 'response),
+    ) => {
+  let parse = request##parse;
+  let parsedHandler =


So the idea around parsedHandler is sort of interesting. Basically, the data that urql returns to you varies based on whether or not you provide a handler to useSubscription. If you don't provide a handler, urql will provide the raw response of executing the subscription (a Js.Json.t that we parse into 'response). If you do provide a handler urql will return whatever data structure you are using to accumulate your subscription data (referred to here as 'acc). parsedHandler, realizing this, parses each subscription as it comes in, and then passes it on to the handler supplied by the user. This ensures 1) proper type inference of subscription responses as they come in, and 2) proper type inference for the data ('acc') returned by useSubscription`. You can see an example of this in the Subscription example.

This is a cool concept, seems like some sort of reducer for subscriptions? Out of curiosity (maybe this is out of scope), how would someone have a subscription that needs to be initialized by a query?
Like if I have an onlineUsers query and some sort of onlineUserUpdate subscription that adds or removes users from that list.

Yep, pretty much! urql allows you to pass a handler and will let you accumulate previous subscriptions with whatever data structure you want. The crux is that, when you pass a handler, urql returns the accumulator you use to maintain data, whereas when you don't, it just returns the raw subscription result, hence the need for two separate hooks here (to get the type inference) right.

In terms of a subscription initialized by a query, the only way I can think to do it is by having the component that calls useQuery higher up in the tree than the one that calls useSubscription, and passing the result of useQuery into the child that can then pass it into useSubscription. @kitten may have other thoughts, I think there's some discussion on this over in the urql issues.

We were thinking of the state being combined. The initial state of the subscription being the queries initial result.

We've purposefully haven't integrated the two because that can also be done with:

a normalised cache

a custom hook wrapping useQuery and use Subscription

or; local component state

So we wanted to leave this choice up to the user, since it'd otherwise feel like we were over stepping the boundary of what should and can be generalised

parkerziegler · 2019-06-19T05:43:02Z

@Schmavery @gugahoa @kiraarghy if any of you have some time to review the latest changes I'd really appreciate it 🙏. I took the guidance of @Schmavery's two PRs #68 and #69 to inform this one, and the type inference should be 💯 here. Let me know thoughts about the additional useSubscriptionWithHandler hook, which is mostly there to reduce polymorphism due to the way urql passes back different data based on whether you call useSubscription with a handler or not.

gugahoa · 2019-06-20T15:16:12Z

This PR got a little bit too big for me to effectively review, is there anywhere I can find you guys online to chat a little bit while reviewing to make sure I don't miss any context?

Meanwhile I'll comment what I gathered so far

gugahoa · 2019-06-20T15:27:20Z

src/UrqlTypes.re

+   `handler` corresponds to the type of the handler function _before_ the latest subscription has been parsed.
+   `parsedHandler` corresponds to the type of the handler function _after_ the latest subscription has been parsed. */
+type handler('acc) =
+  (~prevSubscriptions: 'acc, ~subscription: Js.Json.t) => 'acc;


Shouldn't this be option('acc)?

Suggested change

(~prevSubscriptions: 'acc, ~subscription: Js.Json.t) => 'acc;

(~prevSubscriptions: option('acc), ~subscription: Js.Json.t) => 'acc;

Yep, good catch! The way I understand ~prevSubscriptions, it will always be undefined the first time it runs, so wrapping it in option is correct 👍 Just missed this one.

gugahoa · 2019-06-20T15:33:26Z

src/hooks/UrqlUseSubscription.re

+
+[@bs.module "urql"]
+external useSubscriptionJs:
+  useSubscriptionArgs => array(useSubscriptionResponseJs) =


I believe it's possible do to away with useSubscriptionWithHandlerJs if we change the signature here and below with something like: (~handler: UrqlTypes.handler('acc)=?, useSubscriptionArgs) => array(useSubscriptionResponseJs)

I'm working on an example of how it could work

Ah gotcha, so we'd have one useSubscriptionJs that takes an optional argument for ~handler for when useSubscriptionWithHandler binds to it. I could see that working!

gugahoa · 2019-06-20T15:46:05Z

src/hooks/UrqlUseSubscriptionWithHandler.re

+type useSubscriptionWithHandlerResponseJs('acc) = {
+  fetching: bool,
+  [@bs.optional]
+  data: 'acc,


Possible issue with type safety

Can you explain a little more? I checked the type inference on the two hooks and they appeared to be working as expected. useSubscription will infer the type of data as the result of calling parse on the Js.Json.t returned by urql (written as 'response). useSubscriptionWithHandler will infer the type of the accumulator 'acc you use in your handler function. We also already parse each Js.Json.t subscription coming back from the API before handing it off to handler, so the inference should work as:

Infer the type of 'response from calling parse.

Infer the type of 'acc via the handler function (i.e. does a user supply an array, a Js.Dict, etc. for accumulating new subscriptions).

This piece is somewhat complex tho and I could definitely be missing something. Thanks as always for a thorough review ❤️

You're completely right! I hadn't thought that useSubscriptionWithHandlerJs is safely wrapped through useSubscriptionHandler

Maybe it would be good to comment this for future contributors to take notice? Because it can still be used in a non-type safe manner, but not by the library users, only inside this module. I believe it would be good to refactor a little bit so that the unsafe surface area is minimized

parkerziegler · 2019-06-20T16:18:41Z

@gugahoa agreed that the diff is getting pretty unreadable w/ all this commenting. If it works for you, I'll make any changes you'd like to see by 5pm PST today, we can merge this, and then we can tackle follow up questions in a new PR? We don't currently have any other channels setup online for discussion, tho I am considering getting a Spectrum set up for urql that could also encompass reason-urql.

gugahoa · 2019-06-20T16:45:40Z

@parkerziegler it would be awesome to have a channel where we can discuss a little bit before consolidating the discussion in github comments.

As for changes, how about only merging useSubscription and Subscription, and move useSubscriptionWithHandler to a new PR. What do you think?

parkerziegler · 2019-06-20T16:54:10Z

@gugahoa I'm a little reluctant to split them just because the example uses useSubscriptionWithHandler and it'd be a pain to break it out now. I think I'd prefer to merge both and then open a separate issue where we can chat additional next steps for it. We still need to bring in these type safety checks for the components as well before cutting a v1, so I think we'll have some opportunity to follow up. Is that ok?

gugahoa · 2019-06-20T17:13:33Z

Sure! It's ok for me

Schmavery · 2019-06-20T19:25:46Z

@parkerziegler I just had a last-minute idea re: handler. (I agree that this PR is getting hairy so feel free to ignore it for now).

Assuming a signature of useSubscription that looks something like this:

let useSubscription: (
  ~handler: UrqlTypes.parsedHandler('acc, 'response)=?,
  ~request: UrqlTypes.request('response)) =>
  useSubscriptionResponse('acc);

What if we gave subscriptions a default handler of (_, x) => x? Like an identity function that preserves the type of the subscription. Then the return type would be the same as 'response when you don't provide a handler, and otherwise it would be the return type of handler.

Not 100% sure it will work but it could let us avoid having 2 entrypoints for subscriptions?

gugahoa · 2019-06-20T19:42:20Z

@Schmavery seems like a great idea. I was trying to achieve polymorphic return with GADT, but this approach is a lot simpler and I can see it working.

gugahoa · 2019-06-20T20:43:55Z

From what I gathered, it doesn't seem possible with a helper function:

type request('b) =
  | Box('b);

let barHelper = (~handler: (option('a), 'b) => 'a, ~request: request('b)) => {
  let Box(r) = request;
  handler(None, r)
}

let bar = (~handler: option((option('a), 'b) => 'a)=?, ~request: request('b), ()) => {
  switch (handler) {
  | None => barHelper(~handler=(_, x) => x, ~request)
  | Some(handler_fn) => barHelper(~handler=handler_fn, ~request)
  }
}

The inferred type of bar is:

let bar: (~handler: (option('b), 'b) => 'b=?, ~request: request('b), unit) => 'b = <fun>;

Schmavery · 2019-06-20T22:19:26Z

Hmm, interesting, thanks for trying this. I'm gonna see if I can get something that works but feel free to ignore this for now guys

gugahoa · 2019-06-20T22:33:48Z

@Schmavery @parkerziegler here an example on how we can achieve polymorphic returns for useSubscription https://reasonml.github.io/en/try?rrjsx=true&reason=C4TwDgpgBAThCOBXCBnYAKA5AIwJRQF4AoKKAHygCEB7ADyzwG4ijRIoALAQwDsATADYQYWLgBooOCZgDG+YqQoAJXoIjp01MMACW1HqNzS8hAHySuuAFydVQkVt37Dx6ZZLkoAOWor+Qm25-YXQAfVdJJhY2aEQUCABlRGwUGRgdbT0DTHFI6TlCD3QAPyC1GEC7EJyI2SMoYrgkVGAbJuQ0Blx5c1lmIiFgKAAzampC0nQYqFzp7AlpmQlSqorbYJFc+ag5ZfaWtoQOjDxrHbMoAG8PUkGqOnQYeVgjluZSUhQAdx1gGQ4oOgyvZ8NcPp4-GogatQsMeD11uVYQYfDwIBInjdPD5IUILjAsQBfd5QYkeIijaglYHCAg4qp7V5oAg0egAVm6zEp1NWdN8DIa+2ZrPQACI2aLORSxjyNgRcep0FwZEsoHwuMBLBcwVBvr9-oDlQUdRRUdACOYANrqzUAXQ8FAS1AAtuoBAiBFAAAJQa0arj20iEoiE+qNJnAFkPDm4RhAA

parkerziegler · 2019-06-20T23:43:28Z

@gugahoa @Schmavery I merged this since it was getting pretty darn big, and opened a new issue here for us to discuss API options. Cheers!

kiraarghy self-assigned this Jun 5, 2019

Schmavery reviewed Jun 5, 2019

View reviewed changes

parkerziegler mentioned this pull request Jun 7, 2019

Expose all of urql context #64

Merged

parkerziegler force-pushed the UrqlUseSubscription branch from 8657691 to b39864e Compare June 9, 2019 02:07

kiraarghy marked this pull request as ready for review June 10, 2019 09:55

kiraarghy changed the title ~~UrqlUseSubscription DO NO REVIEW YET~~ UrqlUseSubscription Jun 10, 2019

kiraarghy requested a review from parkerziegler June 10, 2019 09:57

gugahoa suggested changes Jun 10, 2019

View reviewed changes

kiraarghy and others added 8 commits June 17, 2019 21:55

UrqlUseSubscription

b5596b9

1. Created UrqlUseSubscription.

UrqlUseSubscription

03dd328

1. Added rei for useSubscription. 1. Updated UqlSubscription.re to use new syntax. 1. Updated bs-platform to 5.0.4

UrqlUseSubscription

488f73d

1. Started on examples.

Use webpack resolve to point to single urql copy. Update Subscription…

bdb972f

… and useSubscription bindings.

Delete GettingStarted.md

14b90e8

Didn't mean to commit :S

Update src/components/UrqlSubscription.re

76e7119

Co-Authored-By: Gustavo Aguiar <gugahoa@gmail.com>

Update src/components/UrqlSubscription.re

a4f4514

Co-Authored-By: Gustavo Aguiar <gugahoa@gmail.com>

Improve type safety and inference of useSubscription.

8a8c98d

parkerziegler force-pushed the UrqlUseSubscription branch from b17a778 to 8a8c98d Compare June 19, 2019 04:42

Update snapshots.

65a2071

parkerziegler reviewed Jun 19, 2019

View reviewed changes

Last cleanup before review.

aa5c395

gugahoa suggested changes Jun 20, 2019

View reviewed changes

parkerziegler mentioned this pull request Jun 20, 2019

Implement better type inference in components. #70

Closed

Wrap handler prevSubscriptions with option.

273da68

parkerziegler merged commit ba3007b into master Jun 20, 2019

parkerziegler deleted the UrqlUseSubscription branch June 20, 2019 23:39

	<Subscription query variables=None handler=None>
	<Subscription query>

	~handler: option(handler('a, 'b)),
	~handler: handler('a, 'b)=?,

	<SubscriptionJs query ?variables ?handler>
	<SubscriptionJs query variables handler>

	(~prevSubscriptions: 'acc, ~subscription: Js.Json.t) => 'acc;
	(~prevSubscriptions: option('acc), ~subscription: Js.Json.t) => 'acc;

Conversation

kiraarghy commented Jun 5, 2019

Uh oh!

Schmavery Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkerziegler commented Jun 9, 2019

Uh oh!

kiraarghy commented Jun 10, 2019

Uh oh!

gugahoa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kiraarghy commented Jun 11, 2019

Uh oh!

parkerziegler commented Jun 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkerziegler commented Jun 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gugahoa commented Jun 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkerziegler Jun 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parkerziegler commented Jun 20, 2019

Uh oh!

gugahoa commented Jun 20, 2019

Uh oh!

parkerziegler commented Jun 20, 2019

Uh oh!

gugahoa commented Jun 20, 2019

Uh oh!

Schmavery commented Jun 20, 2019

Uh oh!

gugahoa commented Jun 20, 2019

Schmavery Jun 5, 2019 •

edited

Loading

parkerziegler commented Jun 19, 2019 •

edited

Loading

parkerziegler Jun 20, 2019 •

edited

Loading

gugahoa commented Jun 20, 2019 •

edited

Loading

Schmavery commented Jun 20, 2019 •

edited

Loading