search

reasonml-community / reason-apollo-hooks

Deprecated in favor of https://github.com/reasonml-community/graphql-ppx
https://reasonml-community.github.io/reason-apollo-hooks/
MIT License
137 stars 28 forks source link

Apollo Client 3 Mirroring Exploration #122

Closed jeddeloh closed 4 years ago

jeddeloh commented 4 years ago

See #121 and #117. This is an exploration intended for discussion and alignment. Nothing has been tested at this point (#117 is required and it's missing entirely).

Goals

  1. Have a stable release ready ahead of @apollo/client 3.0 and graphql-ppx-re 1.0
  2. Make it easy to review and contribute by mirroring @apollo/client structure
  3. ~Make it easy to consume by mirroring Js module exports~
  4. Make it easy to review and contribute by establishing some style guidelines
  5. Explore patterns of coexisting wtih other modules in the Apollo ecosystem

Style Choices

Module & Submodule Naming

The style I most often see is to prefix submodules with the parent name MyModule_Utils.re (but maybe my selection is biased). Given the many modules and directories contained with @apollo/client this seemed like the sanest strategy for managing module names.

Namespacing

In an ideal world all the @apollo modules could be grouped under the same top-level module. Modules are not extensible...so what can we do? It doesn't seem totally crazy that we recommend that be done manually. If we follow the submodule naming pattern from above, our module should be named Apollo_Client and someone could do this:

/* Apollo.re file */
module Client = Apollo_Client;
module AnotherApolloLibrary = Apollo_AnotherApolloLibrary;

There are other views, of course. Taking some inspiration from yawaramin's article, I found it rather helpful (at least for readability) to use a double underscore to prefix the modules with the namespace I would have liked: Apollo_Client__ApolloClient.

I would much prefer to use Bucklescript's namespacing as this would cut down on the verbosity of file names and not dirty up the global module namespace. However, I wouldn't be able to add things to the namespace module for goal 3 (make it easy to consume). Also, AFAIK, the namespaces are camel-cased from the hypenated bsconfig "name" which would make it impossible to have a namespace of Apollo_Client. Maybe there will be bucklescript namespacing options for submodules in the future.

The other option would be to abandon goal 3 and namespace under ApolloClient and expect people to find stuff in other modules like ApolloClient.React.useQuery, ApolloClient.ApolloClient.make or recommend they make their own module again:

/* Apollo.re file */
include ApolloClient.Index

Raw vs Js Naming

The convention in Bucklescript is to use toJs and fromJs and I personally tend to use Js_ module to tuck all that stuff away. However, Raw is the pattern that has been established with graphql-ppx-re and I think I like it. I've chosen to go with toRaw and fromRaw here, but definitely seems worth a discussion. parse and serialize are also options, but they feel like they have special weight and meaning coming out of graphql-ppx. Maybe if the need for definition is removed, this won't be an issue.

Directory Structure

I mirrored the @apollo/client directory structure exactly. index.js files are should be represented by a module of the parent folder's name. @apollo/client/react/index.js -> Apollo_Client__React.re, for example.

Types

Type definitions also mirror where they are defined in the @apollo/client directory structure exactly. The only wrinkle is that of course we can't model TypeScript extends (at least to my knowledge) so we ignore representing any of those as a separate types.

Subtypes

Sometimes multiple types were required to represent a single type in TypeScript. In order to help make it clear what is a binding to an actual type and what is just needed by Reason, I've taken a similar naming approach to the modules. For instance, Apollo_Client__React_Types.QueryResult.Raw has a type t that uses t_fetchMoreOptions which in turn uses t_fetchMoreOptions_updateQueryOptions.

Prefer Modules for Types

Most types need some sort of conversion one way or the other, so I personally favor wrapping the type up in a module with a type t and the conversion functions inside.

Prefer Annotating the Whole Function Vs. Arguments

I actually have no preference on this, but I went with the approach of annotating the entire function with a type when certain relationships needed to be enforced rather than annotating the arguments and introducing a type in the function arguments themselves. I actually kinda like the latter since you can't mistype it, but I didn't know that syntax for a long time. :)

TypeScript Error

I used Js.Exn.t despite it not having quite the same shape.

Prefer Grouping Raw Stuff

I found putting the Raw/Js stuff in its own module preferrable to scattered throughout the code since it's unlikely to ever be accessed directly. This should make it easier to parse and create interface files as well.

dependencies and peerDependencies

I prefixed these with the namespace, but otherwise approached these like they were separate modules taking the same mirroring approach. Honestly, from a community perspective I'd like to see these as independent projects even if it's just a type t in reasonml-community, but maybe it's not worth it.

Changes to Current Behavior

useMemo

We are using useMemo on the output of hooks and it has no semantic guarantee. From React docs:

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

I really wish they'd called it something else. I think the intent is to have a semantic guarantee rather than a performance optimization where we use it so I added a helper hook that uses refs instead (please check me on this).

Context.t = Js.Dict.t(string)?

I've never used it, but my assumption is this is meant to be anything and not just strings? Anyway, I typed it as Js.Json.t for this pass since I didn't know the effort/value of threading a type variable everywhere.

General Questions/Comments

Binding to Objects

With every Bucklescript release we get better and better options to bind to stuff and I become more and more confused about which one to use. I opted to use straight records here because they're simple and it seems like this is the direction things are going over the long term. However, the typescript bindings explicitly define some objects with optional keys, not just optional values, which we can't model with this. Does it matter? It's javascript, probably not. If we exposed these types to the end user I would lean harder toward a @bs.obj or @bs.deriving approach.

Specific examples:

data: TData | undefined;
error?: ApolloError;

Should we say data: is option('tDAta) since it's specifically undefined? Or should we type as Js.nullable just to be safe? FWIW, I chose to be safe. The same question applies for error, but in the case that we're going in the direction from Reason to Apollo, do we want to approximate {data} (no error) with {data, error: undefined}? I doubt it's going to cause any issues, but it's also not a big deal to use [@bs.obj] or [@bs.deriving abstract] either.

toRaw and fromRaw

It seems one challenge we have is creating new objects when converting types. I don't think this is an issue, but something to be aware of. If we were to, say, wrap the onComplete option passed to useQuery such that it received parsed data, we would be creating a new function on every render. Obviously we could memoize, but I think this complicates things because toRaw for queryOptions would have to become a hook? Again, I don't think this matters in practice, but I don't love it. My strategy on this pass was to not wrap stuff like onComplete. You need to parse it yourself. All the types are there.

Proposal: the properties on the output of a toRaw should always be able to be checked for referential equality?

parse Exception Handling

We are currently catching the parse exception and returning None in that case. I'm sure there are good reasons for this, but a parse failure, given all the type assurance I have, seems like a catastrophic failure I'd personally want to know about. For this pass, I've used straight parse that throws an exception to reduce work until I understand the history of this or we decide on an error handling strategy.

'raw_t_variables

I made the assumption that variables coming in will have already been converted to Raw types through makeVariables especially given the upcoming work around use in graphql-ppx-re.

Uncurrying?

I wonder if its worthwhile to declare any internal stuff that can be uncurried.

Summary

It's pretty ugly! But the Stockholm syndrome is already starting to set in. :) While it's helpful to have the current code for a sanity check, there's enough attention to detail that needs to be paid here, you can't really just copy/paste and tweak some things. I wouldn't commit to this direction lightly.

Sorry for the length of this PR. I just documented whatever decisions I made over the course binding to useQuery and this is the result. It's possible some of these topics are better discussed in an issue than directly in this PR. Anyway, I'm looking forward to hearing if people think this gets at the goals and if it's worth the effort.

jfrolich commented 4 years ago

I like going for reason-promise as a good default, and great to have a way to back out of that decision.

jfrolich commented 4 years ago

It would also be good to have a documentation site. We can reuse the graphql-ppx version. And just change the name logo etc.

Actually thinking about it. This is such a big rewrite, and it's also completely changing the scope of the project. Wouldn't this be better as a separate project and a separate package? (reasonml-communty/reason-apollo-client).

jeddeloh commented 4 years ago

@jfrolich Yeah, I also think this makes more sense as a separate project. I actually just took a pass at what this might look like here: https://github.com/jeddeloh/bs-apollographql I want to make sure everyone is on the same page so we're not splitting the community and unnecessarily introducing even more confusion than already exists, though.

As far as a new project is concerned, I think we might be best served by having a package for each js package we're binding to and publish under one namespace.

That repo is still very rough (I've never used lerna or yarn workspaces), but you can see generally how it turns out in EXAMPLES. There's just enough there to get a basic query going. I went down the path of leveraging bucklescript namespaces—I just found this much nicer to work with. It does require the extra step of aliasing in an Apollo module or whatever you want, but if that's really a problem, we can publish an all-in-one package that doesn't require this. I like that this allows you to have as much granularity I you want. Overall I like it. There are some downsides to this everything-in-its-own-package approach, but not enough to make me switch directions at this point.

Note that right now I'm planning on publishing under my @jeddeloh user namespace while I'm figuring out lerna, but if we want to actually go down the new project route, better publishing options might end up with packages looking like @reasonml-community/bs-apollo--client or @bs-apollo/client. I've used bs- here because I don't think any of this is reason-specific despite leveraging a couple reason-react types, but I didn't think about it much.

jeddeloh commented 4 years ago

It would also be good to have a documentation site. We can reuse the graphql-ppx version. And just change the name logo etc.

Sounds fantastic

jeddeloh commented 4 years ago

Updated based on feedback received so far: https://github.com/jeddeloh/reason-apollo-client

jfrolich commented 4 years ago

We can support graphql-tag natively in graphql-ppx now as it support tagged template literal. Also when we use the tagged template literal there are some performance improvements (precompilation).

jfrolich commented 4 years ago

Some notes:

jfrolich commented 4 years ago

https://github.com/jeddeloh/reason-apollo-client/blob/master/src/%40apollo/client/react/types/ApolloClient__React_Types.re#L33

I would say here that we don't allow passing optional variables because it's not typesafe.

jeddeloh commented 4 years ago

@jfrolich made those changes, but had a couple questions: https://github.com/jeddeloh/reason-apollo-client/pull/1

jeddeloh commented 4 years ago

I'm going to close this pull request now that https://github.com/jeddeloh/reason-apollo-client has reached a pretty good place. Don't hesitate to let me know if any of that could be useful in whatever direction reason-apollo-hooks goes in the future. Thanks!