Apollo Client 3 Mirroring Exploration

[jeddeloh]

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]

Hey @jeddeloh, off to a great start! Here some feedback on some of the questions:

Namespacing

About namespacing. I think not using namespacing, and exposing a single user-facing module ApolloClient, is fine. With the rest as ApolloClient__Query aliased in the library ApolloClient.Query (or whatever the submodules are called). If we use namespacing we cannot expose functions or values to the main module, so I think that is a big downside in the user-facing API, and even packages like reason-react don't use a namespace for that reason.

Raw vs Js

Raw is the raw datatype without conversion. It is used because graphql-ppx also targets native. Also it maps to the JSON GraphQL data and not specifically JavaScript.

The upside is that raw types, parse and serialize all shouldn't specifically be exposed to the user in the main apollo API.

useMemo

I think useMemo is used just as a performance optimization in the library (in order not to parse the data on every render), so I think it's used correctly. If you use refs, you have to invalidate them, and with memo that is done for you with the dependency array.

Rest

I think using records is preferable for mapping to typescript objects. Usually optional keys are compatible with a key that has an undefined value (unless the implementation specifically checks for key existence, if that is the case we can send a PR to apollo-client).

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.

Can you elaborate on this (I don't really get this example).

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.

I think you can even not catch a parse exception, as it is exceptional (it shouldn't happen).

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.

Yes that is a good assumption to make.

I wonder if it's worthwhile to declare any internal stuff that can be uncurried.
I would worry only if these are functions that are called very often in a short amount of time, like in a loop, for it to be a meaningful optimization. Usually, data fetching and parsing, etc. doesn't happen that often.

[jfrolich]

By the way. I think that we don't have to religiously stick to mapping 1-1 to the @apollo/client API. I guess for some patterns we can opt for more idiomatic Reason approaches.

[jfrolich]

Another nice thing that we can make use of is that graphql-ppx can now output tagged template literals. This makes some compile time optimizations available to @apollo/client with graphql-tag.

[jfrolich]

And I would suggest to collaborate tightly so we can offer the best experience. For some things there might need to be some changes/additions to the ppx. I suggest it would be best to release the 1.0 of both graphql-ppx and reason-apollo-hooks at the same time. So it would be the best experience migrating for users.

Naming wise perhaps we can try to get this published under the npm package @reasonml-community/apollo-client?

[fakenickels]

hey there @jfrolich, I haven't been following up close the whole discussion about graphql-ppx@1. I'm catching up right now with everything. About moving the lib to reasonml-community, it is something I have been thinking about lately. As the lib is important for the community, and also used for onboarding Reason newcomers it looked like a good idea! Let's discuss the details about the moving

[jeddeloh]

Namespacing

About namespacing. I think not using namespacing, and exposing a single user-facing module ApolloClient, is fine. With the rest as ApolloClient__Query aliased in the library ApolloClient.Query (or whatever the submodules are called). If we use namespacing we cannot expose functions or values to the main module, so I think that is a big downside in the user-facing API, and even packages like reason-react don't use a namespace for that reason.



Okay, so the proposal is this: keep it like is, but our top-level module would be ApolloClient not Apollo_Client? Sounds good to me 👍

[jeddeloh]

Raw vs Js

Raw is the raw datatype without conversion. It is used because graphql-ppx also targets native. Also it maps to the JSON GraphQL data and not specifically JavaScript.
The upside is that raw types, parse and serialize all shouldn't specifically be exposed to the user in the main apollo API.

I was excited about having more agnostic language around conversion at the edges, but reading this, I’m now back to thinking we should stick with toJs and fromJs.

[jeddeloh]

useMemo

I think useMemo is used just as a performance optimization in the library (in order not to parse the data on every render), so I think it's used correctly. If you use refs, you have to invalidate them, and with memo that is done for you with the dependency array.

Agreed on the performance/parsing part, but looking through some of the PRs/issues that led to the addition of useMemo it looked like people were specifically looking to use the output in useEffect. If it were critical that someone's effect were run once and only once for each change, we force users to be very careful about the values they check against since they can't rely on useMemo, don’t we?

FWIW, if I’m understanding you correctly, the useGuaranteedMemo hook does do the ref invalidation (I think we only ever have one dependency). I had this same discussion over in reason-urql and now I’m beginning to feel like I just have some huge misunderstanding about useMemo and am being an annoyance about something that doesn’t matter. 😅If people are confident useMemo is what we want here, I won't press the issue.

[jeddeloh]

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.

(jfrolich): Can you elaborate on this (I don't really get this example).

I was just trying to point out that if we're recreating functions on every render that are passed into the Js Apollo Client (onComplete option in this example), we have to be okay with whatever internal re-computations occur there. I think it's not a big deal, but was trying to get at some conceptual purity of toJs. I'm now of the opinion I shouldn't have brought it up. I think we can ignore it.

[jeddeloh]

I think you can even not catch a parse exception, as it is exceptional (it shouldn't happen).

I think that is also my preferred error handling strategy here.

[jfrolich]

Ah ok. You are right that you can’t necessarily be certain it changed when you use it in useEffect as a dependency. I don’t really know when that should be an issue though? But I might be missing something!

[jeddeloh]

By the way. I think that we don't have to religiously stick to mapping 1-1 to the @apollo/client API. I guess for some patterns we can opt for more idiomatic Reason approaches.

Actually, this is a big one. I completely agree here and think we should remove that as a goal and struck it through in the description.

However, it exposes to me the true reason I wanted it in the first place. I feel like reason libraries mix two concerns: binding to the Js library, and the extra, reason-specific library code around those bindings. It’s the Js library bindings that I’m really proposing have a 1:1 mapping. Those bindings don’t have to be complete, but the idea is that you should be able to describe some repeatable patterns anyone could use to contribute to bindings in a consistent way.



Because those two concerns are mixed might be one of the reasons the community is littered with abandoned half-finished bindings to libraries I rarely feel like contributing to. If AstroCoders/reason-apollo-hooks moves to reasonml-community this might be an opportunity to iterate toward better patterns. I propose we think about the possibility of publishing three reason packages @bindings/graphql, @bindings/apollo-client, and @reasonml-community/apollo-client (npm namespaces made up). Maybe that’s unrealistic, I haven’t thought that much about it yet, but would love to hear if there are any thoughts.

[jeddeloh]

Another nice thing that we can make use of is that graphql-ppx can now output tagged template literals. This makes some compile time optimizations available to @apollo/client with graphql-tag.

And I would suggest to collaborate tightly so we can offer the best experience. For some things there might need to be some changes/additions to the ppx.

Agreed and very excited about this! But I’m deferring entirely to the maintainers at this point.

[jeddeloh]

Ah ok. You are right that you can’t necessarily be certain it changed when you use it in useEffect as a dependency. I don’t really know when that should be an issue though? But I might be missing something!

I had this same question! I haven't had a need for this since I've been able to be very specific with effect dependencies, especially with ones I need to ensure run only once. However, context from an issue here about mutations, and from reason-urql is that people have the expectation to be able to use the output here to trigger hooks in a reliable way. Having had this discussion, I'm personally fine with useMemo but I want to make sure we're not introducing booby-traps for people. 🤷

[jeddeloh]

@fakenickels, any thoughts on how to proceed here? I don't mean to rush anyone, but I've been (perhaps incorrectly) waiting on confirmation from you if this general direction looks good before moving further. After that, next steps might look like:

1. Get an apollo-client-3 (name?) branch up
2. Add an apollo-client-3 tag for issues
3. Merge Make compatible with GraphQL PPX 1.0 beta #117 into apollo-client-3
4. Finish up loose ends here and merge
5. Make small PRs into apollo-client-3 going forward

I can also just do everything in this PR if you'd rather.

[fakenickels]

Hey there @jeddeloh! Thanks for working on this. We were figuring out the lib move to reasonml-community. Now we are here finally!

[fakenickels]

Create a new branch and keeping pushing there along with a npm beta tag for it (as @apollo/client@3 is still too a beta) seems the way to go. I still need to take a better look at the new modifications of the client API.

As the lib is now at reasonml-community, creating a separated package or turning this one into a monorepo which would include the hooks lib and the client one seems to be a more maintainable approach.

[fakenickels]

Okay, so the proposal is this: keep it like is, but our top-level module would be ApolloClient not Apollo_Client? Sounds good to me 👍

ApolloClient for sure

I was excited about having more agnostic language around conversion at the edges, but reading this, I’m now back to thinking we should stick with toJs and fromJs.

toJs and fromJS as well

[fakenickels]

this also would be called JS type right? maybe we could add some comments explaining a bit more to help future contributors

[jeddeloh]

Yes. This would be JS or Js_. Now that you bring this up, I think it's possible it would be clearer to future contributors if there was a clear 1:1 Bindings module rather than mixing this stuff, but who know. I'll try a branch off this PR and see if people like that direction.

[jeddeloh]

Re: explaining a bit more to help future contributors, I think a CONTRIBUTING.md with some explanation of patterns used might be a good goal. https://help.github.com/en/github/building-a-strong-community/setting-guidelines-for-repository-contributors

[fakenickels]

what this means?

[jeddeloh]

It's just saying that in typescript WatchQueryFetchPolicy only defines one value and the rest are through extending FetchPolicy. I usually try to keep properties in alphabetical order, but here I was experimenting to see if it's more useful to separate each group by what is extended. Maybe we should ignore it or make it clear what the comment is saying 😆

[fakenickels]

I usually prefer to use bs.deriving for this
https://bucklescript.github.io/docs/en/generate-converters-accessors#usage-5

[jeddeloh]

Nice! I prefer this as well, but for some reason I thought bs.deriving only worked on polymorphic variants and that this was an intentional style decision before.

[jeddeloh]

Well, nevermind, I got excited and misread the docs. You do indeed need polymorphic variants, but it sounds like that's the direction we want to go (and I agree), so I'll still make the switch.

[fakenickels]

I think jsData could be easier to read and grasp for future contributors reading this

[jeddeloh]

Agreed.

[jfrolich]

About the hooks and client as separate packages. I am in favor of having a single package because that is also equivalent to the @apollo/client package, and it makes maintenance easier (people don't need to keep two libraries in sync).

[jfrolich]

@jeddeloh In the main time I am working on getting #117 working with the new graphql-ppx API that was discussed and decided on last week.

[jeddeloh]

Hey there @jeddeloh! Thanks for working on this. We were figuring out the lib move to reasonml-community. Now we are here finally!

Nice! Thanks for doing that!

[jeddeloh]

@jeddeloh In the main time I am working on getting #117 working with the new graphql-ppx API that was discussed and decided on last week.

@jfrolich Sounds great. Is this just switching the hooks to take the FCM self instead of definition or does it include the "extends" API as well?

My plan is to update this PR to account for comments, then experiment quickly with putting the js bindings in their own module, and finally move on to ApolloClient (meaning the actual class, not this library) bindings because they shouldn't be affected at all by the new graphql-ppx API.

[jeddeloh]

I'm leaning toward wrapping all our stuff in a try block and converting exceptions to an ApolloError. Thoughts? I have definitely worked with backends that have problems and parse errors were not uncommon.

[jfrolich]

@jeddeloh: In parallel I am trying to get most reason-apollo functionality and a bit more in my PR (basically syncing my PR with the custom bindings we have). So you can have a look and reuse. Especially the Observable bindings were a bit tricky to get right.

[jeddeloh]

Status update:

• With the addition of bindings for ApolloClient creation and @apollo/link-ws, the branch is in a good enough state to support me on personal projects, but there is a ton here that may be forever untested by me. 🤷 I've included a basic client example if anyone wants to help or play around.
• Basic hook bindings are still missing: useMutation useClient useLazyQuery, etc. They should be a comparatively small effort now that a lot of types are in, though.
• Legacy hooks are still missing
• Any common links besides link-ws that are not in the @apollo/client package are missing bindings
• I've experimented with trying to convert exceptions to a result record in useQuery just to see how bad it would be. It seems problematic for a number of reasons, but I'm gonna keep it there for a sec.
• Side note: using option on a record property to represent a property that may not exist did indeed bite me for InMemoryCache as well as when the variables type coming out of graphql-ppx is unit. Some(()) does not compile to undefined :) It looks like we might get more annotations, but it's worth noting you should be very careful when doing this.

[jeddeloh]

At this point I'm tempted to start exploring the configuration idea as mbirkegaard suggested for context. Personally, and when considering new users, I would love this library to provide some sane promise transforms out of the box. We could chose reason-promise by default and people can just provide an identity function for promiseTransform if they want to stick with vanilla Js promises, or could use a stream library or whatever they want.

[jfrolich]

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

[jfrolich]

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]

@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]

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]

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

[jfrolich]

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]

Some notes:

• ExtendNoRequiredVariables. This will be the extension when there are no required variables, so there is a makeDefaultVariables function available in the module. We might still want to pass variables, so variables should still be an optional argument there.

• useMutation (not implemented yet but FYI). useMutation offers you to pass in variables in the hook and in the function that the hook returns. This is not possible to make typesafe. So in my implementation I created two versions of useMutation:

  • useMutation: returns a function that requires a variables argument
  • useMutationVariables: pass in variables in the hook, returns a function that has no variables argument
  • useMutation0: when there are no required arguments, has no variables argument

[jfrolich]

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]

@jfrolich made those changes, but had a couple questions: jeddeloh/rescript-apollo-client#1

[jeddeloh]

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!