Proposed API change corrections #59
Conversation
This reverts commit 780c003.
|
Thanks a lot for your work! I wonder whether it would be possible to do the file rename in another PR? It would make it so much easier to review if we could compare with the previous useQuery and useMutation 🙂 Just a suggestion, what do you and others think? Would love some input from other reviewers! I think we will need to rethink the incompleteMutation approach, it might be confusing for people just starting to use the library. I will experiment with it a bit more during the weekend 🙂 |
Indeed. I couldn't find a better way without changing the javascript code. Looking forward to your input! |
Because of the revert-revert I don't know of a way to do this. Any ideas? At least to see the diff it is still possible to diff the two different files using a diff tool. |
Indeed! I think was just a problem with git itself, the change was so big it couldn't detect automatically that the files are still the same bot moved and changed. I was talking with @MargaretKrutikova and what I'm gonna do is to create .rei files for the current API so we can detect better the breaking changes or regressions |
FYI Me too! |
|
I actually found a way to remove the need of |
Good one. But since the whole API changed it's going to be a large breaking change. As far as options it probably good to go through the changes after the first commit of this PR (September 18), and see everything has been changed in this branch as well. |
|
This is the diff output of index f9d071d..b9f722c 100644
--- a/README.md
+++ b/README.md
@@ -120,6 +120,26 @@ Using `fetchPolicy` to change interactions with the `apollo` cache, see [apollo
let (_simple, full) = UserQuery.use(~fetchPolicy=NetworkOnly, ());
```
+Using `errorPolicy` to change how errors are handled, see [apollo docs](https://www.apollographql.com/docs/react/api/react-apollo/#optionserrorpolicy).
+
+```reason
+let (simple, _full) = UserQuery.use(~errorPolicy=All, ());
+```
+
+Using `skip` to skip query entirely, see [apollo docs](https://www.apollographql.com/docs/react/api/react-apollo/#configskip).
+
+```reason
+let (simple, _full) =
+ UserQuery.use(
+ ~skip=
+ switch (val) {
+ | None => true
+ | _ => false
+ },
+ (),
+ );
+```
+
## useMutation
```reason
diff --git a/commitlint.config.js b/commitlint.config.js
new file mode 100644
index 0000000..e89caf4
--- /dev/null
+++ b/commitlint.config.js
@@ -0,0 +1,32 @@
+module.exports = {
+ rules: {
+ 'body-leading-blank': [1, 'always'],
+ 'footer-leading-blank': [1, 'always'],
+ 'header-max-length': [2, 'always', 72],
+ 'scope-case': [2, 'always', 'lower-case'],
+ 'subject-case': [
+ 2,
+ 'never',
+ ['sentence-case', 'start-case', 'pascal-case', 'upper-case']
+ ],
+ 'subject-empty': [2, 'never'],
+ 'subject-full-stop': [2, 'never', '.'],
+ 'type-case': [2, 'always', 'lower-case'],
+ 'type-empty': [2, 'never'],
+ 'type-enum': [
+ 2,
+ 'always',
+ [
+ 'build',
+ 'chore',
+ 'ci',
+ 'docs',
+ 'feat',
+ 'fix',
+ 'perf',
+ 'refactor',
+ 'revert'
+ ]
+ ]
+ }
+};
diff --git a/package.json b/package.json
index f1e8f21..1ebfdd9 100644
--- a/package.json
+++ b/package.json
@@ -1,6 +1,6 @@
{
"name": "reason-apollo-hooks",
- "version": "2.5.1",
+ "version": "2.6.0",
"scripts": {
"build": "bsb -make-world",
"start": "bsb -make-world -w",
@@ -14,7 +14,11 @@
"license": "MIT",
"devDependencies": {
"@apollo/react-hooks": "^3.0.0",
+ "@commitlint/cli": "^8.2.0",
+ "@commitlint/config-conventional": "^8.2.0",
"bs-platform": "^5.0.6",
+ "husky": "^3.0.5",
+ "lint-staged": "^9.4.0",
"reason-apollo": "^0.17.0",
"reason-react": ">=0.7.0"
},
@@ -24,5 +28,17 @@
"react-dom": "^16.8.6",
"reason-apollo": "^0.17.0",
"reason-react": ">=0.7.0"
+ },
+ "lint-staged": {
+ "*.{re,rei}": [
+ "bsrefmt --in-place -w 80",
+ "git add"
+ ]
+ },
+ "husky": {
+ "hooks": {
+ "pre-commit": "lint-staged",
+ "commit-msg": "commitlint -E HUSKY_GIT_PARAMS"
+ }
}
}
diff --git a/src/Mutation.re b/src/Mutation.re
index 4322878..33cb2af 100644
--- a/src/Mutation.re
+++ b/src/Mutation.re
@@ -9,7 +9,7 @@ type graphqlErrors;
type error = {
.
"message": string,
- "graphlErrors": graphqlErrors,
+ "graphqlErrors": graphqlErrors,
};
type refetchQueries =
@@ -29,7 +29,7 @@ type controlledResult('a) = {
error: option(error),
};
-type controledVariantResult('a) =
+type controlledVariantResult('a) =
| Loading
| Called
| Data('a)
@@ -72,7 +72,7 @@ module Make = (Config: Config) => {
~awaitRefetchQueries: bool=?,
unit
) =>
- Js.Promise.t(controledVariantResult(Config.t));
+ Js.Promise.t(controlledVariantResult(Config.t));
[@bs.module "@apollo/react-hooks"]
external useMutation:
diff --git a/src/Query.re b/src/Query.re
index 3c4a786..a0595e6 100644
--- a/src/Query.re
+++ b/src/Query.re
@@ -61,6 +61,12 @@ module Make = (Config: Config) => {
notifyOnNetworkStatusChange: bool,
[@bs.optional]
fetchPolicy: string,
+ [@bs.optional]
+ errorPolicy: string,
+ [@bs.optional]
+ skip: bool,
+ [@bs.optional]
+ pollInterval: int,
};
[@bs.module "@apollo/react-hooks"]
@@ -84,6 +90,9 @@ module Make = (Config: Config) => {
~client=?,
~notifyOnNetworkStatusChange=?,
~fetchPolicy=?,
+ ~errorPolicy=?,
+ ~skip=?,
+ ~pollInterval=?,
(),
) => {
let jsResult =
@@ -94,6 +103,9 @@ module Make = (Config: Config) => {
~client?,
~notifyOnNetworkStatusChange?,
~fetchPolicy=?fetchPolicy->Belt.Option.map(Types.fetchPolicyToJs),
+ ~errorPolicy=?errorPolicy->Belt.Option.map(Types.errorPolicyToJs),
+ ~skip?,
+ ~pollInterval?,
(),
),
);
@@ -133,14 +145,18 @@ module Make = (Config: Config) => {
[|jsResult|],
);
- (
- switch (result) {
- | {loading: true} => Loading
- | {error: Some(error)} => Error(error)
- | {data: Some(data)} => Data(data)
- | _ => NoData
- },
- result,
- );
+ let simple =
+ React.useMemo1(
+ () =>
+ switch (result) {
+ | {loading: true} => Loading
+ | {error: Some(error)} => Error(error)
+ | {data: Some(data)} => Data(data)
+ | _ => NoData
+ },
+ [|result|],
+ );
+
+ (simple, result);
};
-};
\ No newline at end of file
+};
diff --git a/src/Types.re b/src/Types.re
index fe7d418..fa7c38a 100644
--- a/src/Types.re
+++ b/src/Types.re
@@ -11,7 +11,7 @@ type networkStatus =
| Error
| Unknown;
-let toNetworkStatus = (status: Js.Nullable.t(int)) => {
+let toNetworkStatus = (status: Js.Nullable.t(int)) =>
switch (status->Js.Nullable.toOption) {
| Some(1) => Loading
| Some(2) => SetVariables
@@ -22,24 +22,39 @@ let toNetworkStatus = (status: Js.Nullable.t(int)) => {
| Some(8) => Error
| _ => Unknown
};
-};
/**
* apollo-client/src/core/watchQueryOptions.ts
*/
type fetchPolicy =
| CacheFirst
+ | CacheAndNetwork
| NetworkOnly
| CacheOnly
| NoCache
| Standby;
-let fetchPolicyToJs = fetchPolicy => {
+let fetchPolicyToJs = fetchPolicy =>
switch (fetchPolicy) {
| CacheFirst => "cache-first"
+ | CacheAndNetwork => "cache-and-network"
| NetworkOnly => "network-only"
| CacheOnly => "cache-only"
| NoCache => "no-cache"
| Standby => "standby"
};
-};
+
+/**
+ * apollo-client/src/core/watchQueryOptions.ts
+ */
+type errorPolicy =
+ | None
+ | Ignore
+ | All;
+
+let errorPolicyToJs = errorPolicy =>
+ switch (errorPolicy) {
+ | None => "none"
+ | Ignore => "ignore"
+ | All => "all"
+ }; |
|
Could you remove the diff from |
|
First of all i think changes in this PR are greate, it will significantly simplify usage.
I noticed that |
Done |
I forgot to save before committing. Resolved with the last commit. |
|
Actually I have an idea to have two useMutation functions, one with a fixed mutation and one that allows the mutation to be passed to the resulting function. This allows the type system to better validate the arguments. |
|
Now with |
|
About useDynamicMutation - it returns a function without any query assign. For example you name the function: setUser, and pass to it something diffrent (for example adress) and it will work. |
README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| If you don't know the value of the variables you can initialize with the query property |
There was a problem hiding this comment.
About useDynamicMutation - it returns a function without any query assign. For example you name the function: setUser, and pass to it something diffrent (for example adress) and it will work.
Im just wondering if it will be good ?
In react apollo you pass query to useMutation and it returns function which only takes variables, so here there is no possible to do mismatch which i wrote above.
Actually in apollo you can pass in a different mutation as options. I agree it's better to pass in the mutation once, and then only change variables, but they are conflated in graphql_ppx. I think for the longer term it's a good idea to suggest a PR to graphql_ppx to separate them (such as Mutation.makeVariables(~id) and Mutation.makeMutation() (that creates the same data structure as make, but without variables -- we need the parse function in that data structure).
There was a problem hiding this comment.
Maybe it's indeed better to future proof the useDynamicMutation API, to pass in Mutation.query as the first argument and a required named argument of variables of Mutation.make(~id)##variables. Then make that API prettier eventually by PR-ing graphql_ppx. What do you think?
Edit: That doesn't work -- we need the parse function that the Mutation.make data structure yields.
There was a problem hiding this comment.
Another option is something similar what you made before: pass Mutation.query to useDynamicMutation and function which is returned can take Mutation.make(~id) (not only variables). But maybe its better to leave it as is and wait for better graphql_ppx.
src/ApolloHooksMutation.re
Outdated
|
|
||
| let useMutation = | ||
| ( | ||
| mutation, |
There was a problem hiding this comment.
Why here mutation is not labeled? In function which is returned by useDynamicMutation, parameter: mutation is labeled.
There was a problem hiding this comment.
Following the API of Apollo, and making it more consistent with useQuery. Maybe the for the returned function of useDynamicMutation the mutation should also be an unlabeled first argument indeed.
There was a problem hiding this comment.
See unlabeled mutation in new commit
There was a problem hiding this comment.
See unlabeled mutation in new commit
Ok, README.md - needs to update ;)
and in my opinion useQuery now should take first argument unlabeled.
There was a problem hiding this comment.
Agree, didn't realized I hadn't refactored useQuery yet.
|
Are we ready to merge this @fakenickels, @MargaretKrutikova, @kamilkuz? 🚀 |
|
hey there @jfrolich will finish the final review today! sorry the delay |
|
saw that your PR to graphql_ppx_re got merged too @jfrolich that's really nice, we can totally explore that now as well. I'm a bit busy today in the morning and partially in the afternoon but I'll do my best to get this into |
|
@Schmavery about the FCM discussion it was getting a bit philosophical for me, if it really adds more burden to beginners or if we are keeping them away from a cool lang feature haha. |
|
I had a chance to hack a bit with @Schmavery yesterday and we made a small (+18LoC) addition to Here's an example of the API differences: And the definition-based api applied to the other operation types: teamwalnut/rescript-urql#133 (comment) let (response, refetch) = Hooks.useQuery(MyQuery.definition, ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation(MyMutation.definition, ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation(MyMutation.definition);
let (response) = Hooks.useSubscription(MySub.definition, ~var=1, ());This way we can avoid introducing any advanced type concepts, the api is (just about) as succinct and intuitive as possible, and it should compile down to (mostly) readable JS. |
Love this! Especially the currying done in composeVariables, was already thinking if such a solution would be possible with currying so we don't need makeVariables in the API. @fakenickels, what do you think? I think it might be good to refactor to above API. |
Actually I think passing the module would allow for a nicer compilation to javascript, and it allows for adding more data to the module later without breaking changes. (For instance the new serialize function). Then we only need What about this api? (@sgrove, any other downsides to passing a (first class) module that I am not aware of?, it compiles really cleanly to JS). I think many people might not know about the particular language feature, but the syntax is pretty clear and you don't need to know all the details to be able to use it. let (response, refetch) = Hooks.useQuery((module MyQuery), ~var=1, ());
// With currying, the ~var=1 can be applied later at the call-site.
let (response, refetch) = Hooks.useMutation((module MyMutation), ~var=1, ());
let (response, execute) = Hooks.useDynamicMutation((module MyMutation));
let (response) = Hooks.useSubscription((module MySub), ~var=1, ()); |
|
Hmm, thought some more about this, but there are extra parameters to pass into So all things considered in my opinion the API as in this PR is better for Apollo. I think the URQL API is also interesting, especially for a light implementation with a very simple API (and all the advanced options that Apollo offers). We can consider to also pass in definition tuple instead of the (first class) module. I don't really care, I think both options are fine. |
I was wrong we can mix the curried labeled arguments, so actually if the options don't collide with the variables, this might be a quite elegant solution. Experimenting with it right now but running into some issues with the type system. |
|
As it looks now the API as proposed by @sgrove has a side effect that the variables have to be provided exactly in the order of the query. I reproduced this in their PR (teamwalnut/rescript-urql#133 (comment)). This is an important downside to be aware of. It makes the DX worse as people will not know why they get type errors for labeled arguments that normally can be in any position. Let's see if there is a solution to that. I don't completely grok why it is, but it's probably because of the polymorphic parametric types that the compiler doesn't have full information of the return type, and thus enforces the order of parameters. Any help appreciated of people that are more familiar with the intricacies of this! |
Does it mean that if we use
I totally agree! I really like your addition with So maybe we can try out passing definition tuples instead of modules and leave
|
I was wrong about it and it can be mixed with other labeled arguments, so that API would be pretty sweet. The only thing is that I found that the compiler doesn't accept the variables out of order due to the currying (I think). I pointed this out in the PR (teamwalnut/rescript-urql#133 (comment)).
I get the impression most people prefer I'll wait for a bit if we can get the composeVariables currying working correctly. If that's technically not possible I will refactor to:
|
|
Both APIs look equally good and now the discussion is more about aesthetics so far, as I think no one could see clear disadvantages about one or the other. So @jfrolich I think we could merge the PR as it is and let others use both libraries (urql and apollo-hooks) with this difference and see how it goes in the future. Probably a bit of difference will be good for now. What you think? cc @MargaretKrutikova @kamilkuz Another point is to check if the demo is still working, can't trust hooks without checking in runtime unfortunately |
|
I like api with |
|
Hm... I really think we should go with I have made this switch to |
|
Awesome @MargaretKrutikova. I think I have a slight preference to |
|
Agree with @MargaretKrutikova ! Indeed better keep things consistent across the ecosystem |
|
Are we ready to merge? 😄 |
|
I'm AFK now but will review tonight later and we can merge 💃 |
|
Looking good. |
|
Available now under v3.0.0 |
Reverting the reverted commit (Work in progress)
Let me know if there is anything else to fix as this is a pretty big change to the API surface.
Previous PR: #49