✨ add `useQuery` hook by michenly · Pull Request #663 · Shopify/quilt

michenly · 2019-04-18T20:10:40Z

Re-open #642
Close #586

🎩 instructions:

get this branch on web-proving-ground locally
run yarn in web-proving-ground
in quilt, do yarn && yarn tophat react-async ../web-proving-ground
again in quilt, do yarn tophat react-hooks ../web-proving-ground
again in quilt, do yarn tophat react-graphql ../web-proving-ground
once it complied, go to web-proving-ground and do yarn dev
goto localhost:8081 to see the query & mutation working properly

^ kinda cool I can do tophat with two different lib. As long as I do the tophat in the dependency order.

michenly · 2019-04-22T21:38:48Z

packages/react-graphql/src/hooks/index.ts

@@ -1,4 +1,4 @@
-export {ApolloProvider} from './ApolloProvider';
-export {useApolloClient} from './apollo-client';
+export {default as useApolloClient} from './apollo-client';


decided to export this so we can use it instead of withApollo

lemonmade

Overall looks great 👍

lemonmade · 2019-04-23T18:41:25Z

packages/react-async/src/Async.tsx


 import {AsyncAssetContext, AsyncAssetManager} from './context/assets';
-import {normalize, resolve} from './utilities';
+import {initialResolved, resolve} from './utilities';


I think maybe trySynchronousResolve provides the explicitness that we kind of want here, given that it is a bit of an odd process

lemonmade · 2019-04-23T18:42:30Z

packages/react-async/src/utilities.ts

+}
+
+export function initialResolved<T>({id, defer}: Options<T>): T | null {
+  const canResolve = defer == null && id;


I think defer can also be DeferTiming.Mount and we'd still want to do this

lemonmade · 2019-04-23T18:42:45Z

packages/react-async/src/utilities.ts

+
+export function initialResolved<T>({id, defer}: Options<T>): T | null {
+  const canResolve = defer == null && id;
+  const resolved = canResolve && id ? tryRequire(id()) : null;


&& id is not needed given the check above, so remove it from one

lemonmade · 2019-04-23T18:43:23Z

packages/react-graphql/README.md


+### `useApolloClient`
+
+`useApolloClient` hook can be use to access apollo client that is currently in the context.


What do you think about calling it useGraphQLClient instead? I am not sure if it's better to hide Apollo's involvement or not here...

we do call this lib react-graphql ...
I can go both way of this, useApolloClient make it so people know exactly they are getting the client object from apollo and should look up apollo API on how to use it.

useGraphQLClient hides that, but is future proof if we ever not use apollo?

Who knows about the Apollo thing, I think this is fine though (renaming a function is a fairly easy refactor later anyways) 👍

lemonmade · 2019-04-23T18:43:33Z

packages/react-graphql/README.md

+### `useApolloClient`
+
+`useApolloClient` hook can be use to access apollo client that is currently in the context.
+Which can be use to trigger query manually.


Not really a sentence

lemonmade · 2019-04-23T18:47:16Z

packages/react-graphql/src/async.tsx

+  Object.defineProperty(FinalComponent, 'resolved', {
+    get: () => resolvedDocument,
+    // accessor descriptor don't need this
+    // writable: false,


lemonmade · 2019-04-23T18:48:47Z

packages/react-graphql/src/hooks/graphql-document.ts

+    async () => {
+      if (!isDocumentNode(documentOrComponent)) {
+        try {
+          const resolved = await documentOrComponent.resolve();


I think we need to check whether the component is mounted before doing this? I wrote a nice hook for this we can throw in the shared package:

export function useMounted() { const mounted = useRef(true); useEffect(() => { return () => { mounted.current = false; }; }, []); return mounted; }

arg...I would have assume that react have this behaviour imbedded in all the hooks now. (don't know why I thought that...)

@lemonmade curious as to why you are using a ref in the hook above instead of a variable? I can see how useRef can be useful if the value being assigned was an object since we can re-use the same reference without creating new object references, however, if it is a boolean, it should be fine to use let mounted ?

I think we need to check whether the component is mounted before doing this?

I likely don't have context, but why do we need to check for mounted ? Won't this function useGraphQLDocument not called at all if the component using it is not mounted? Feels like I am missing context 🤔

Yes, the other option here is just to let mounted = true;, then set it to false in the callback, then check that variable in scope after the async operation. The hook just wraps that logic up (the only way to wrap it up in a hook is to do it with a ref, you can't really store a persisted value any other way).

Yes, useEffect only gets called when the component is mounted, but that doesn't mean it will end while the component is still mounted. If you kick off an async operation (as is done here), there is no requirement for React to wait for it to be done while the component is still mounting (it would be catastrophic if the UI didn't update until the promise resolved, and it would make no sense to keep the component mounted beyond when a parent unmounts it).

so what to know is this setState is after an await
the component will be mount when the async method start, but may not be mounted by the time it finished.

Ref is normally uses for accessing the dom, but in hooks land, it is also the react recommended way to keeping value you will normally keep around using property variable in class component.

Since hook turn the function itself into a component that has lifecycle, just using a variables that can be access through out the different hook can have unexpected result.

The is literally the exact same problem solved by having a mounted instance variable on a class component.

Makes sense. Thanks to you both!

As for the mounted ref, an ideal solution would have been to have some kind of cancel/abort for loadDocument which we return from the useEffect so that it cancels the document load if the component unmounts. Having said that, I can't think of a reliable way to do so for this use case.

Yes, it would definitely be preferable, we can potentially wrap in a cancellable promise as an alternative (I suggested a mounted ref because I had already written it, a cancellable promise is a little trickier)

packages/react-graphql/src/hooks/query.ts

lemonmade · 2019-04-23T18:55:22Z

packages/react-graphql/src/hooks/query.ts

+    [skip, queryObservable],
+  );
+
+  const previousData = useRef<QueryHookResult<Data, Variables>['data']>();


Don't need an initial value here?

lemonmade · 2019-04-23T18:56:44Z

packages/react-graphql/src/hooks/tests/query.test.tsx

+
+  describe('document', () => {
+    it('returns loading=true and networkStatus=loading during the loading of query', async () => {
+      const MockQuery = ({children}) => {


Function declarations :D

michenly · 2019-04-24T22:17:26Z

Still have the following to do:
but useQuery itself can be re-review

Todo:

lemonmade

Very happy with this 👍

lemonmade · 2019-04-25T14:14:05Z

packages/react-async/src/Async.tsx

 function initialState<Value>(props: Props<Value>): State<Value> {
-  const canResolve = props.defer == null && props.id;
-  const resolved = canResolve && props.id ? tryRequire(props.id()) : null;
+  const resolved = trySynchronousResolve<Value>({


FWIW, you can usually just pass props to this function directly, it has more properties but that's fine because it matches the type signature

lemonmade · 2019-04-25T14:15:03Z

packages/react-graphql/README.md


+### `useApolloClient`
+
+`useApolloClient` hook can be use to access apollo client that is currently in the context.


Who knows about the Apollo thing, I think this is fine though (renaming a function is a fairly easy refactor later anyways) 👍

lemonmade · 2019-04-25T14:15:25Z

packages/react-graphql/README.md

+}
+```
+
+### with query document


For follow up: make sure headings are sentence case

lemonmade · 2019-04-25T14:21:38Z

packages/react-graphql/src/hooks/graphql-document.ts

+    } else {
+      return documentOrComponent.resolved;
+    }
+  });


I wonder if this is a case where we can break the rule of hooks by short circuiting here. Maybe better as an optimization later, but I think it will at least be an option.

lemonmade · 2019-04-25T14:22:16Z

packages/react-graphql/src/hooks/graphql-document.ts

+        loadDocument();
+      }
+    },
+    [document, documentOrComponent, loadDocument],


Don't think you need documentOrComponent here?

lemonmade · 2019-04-25T14:24:29Z

packages/react-graphql/src/hooks/types.ts

+export interface QueryHookResult<Data, Variables>
+  extends Omit<QueryResult<Data, Variables>, 'networkStatus' | 'variables'> {
+  networkStatus: QueryResult<Data, Variables>['networkStatus'] | undefined;
+  variables: QueryResult<Data, Variables>['variables'] | undefined;


We can technically offer up better typings for some of these because we can check if the variables have any required keys or not, but for now this is fine 👍

michenly force-pushed the use-query branch 3 times, most recently from 8944d5c to 248b773 Compare April 22, 2019 17:53

michenly marked this pull request as ready for review April 22, 2019 17:54

michenly requested review from GoodForOneFare, cartogram, lemonmade and marutypes April 22, 2019 17:54

michenly self-assigned this Apr 22, 2019

michenly force-pushed the use-query branch 2 times, most recently from 958baf5 to 9ddaf5a Compare April 22, 2019 21:22

michenly commented Apr 22, 2019

View reviewed changes

lemonmade suggested changes Apr 23, 2019

View reviewed changes

michenly force-pushed the use-query branch 6 times, most recently from 6895705 to 5da5726 Compare April 24, 2019 22:16

michenly requested a review from lemonmade April 24, 2019 22:19

michenly force-pushed the use-query branch 2 times, most recently from 7af9df9 to ec53193 Compare April 25, 2019 13:43

lemonmade approved these changes Apr 25, 2019

View reviewed changes

michenly force-pushed the use-query branch from ec53193 to 5e0bec9 Compare April 25, 2019 14:47

✨ add useQuery hook

bbb05d6

michenly force-pushed the use-query branch from 5e0bec9 to bbb05d6 Compare April 25, 2019 15:10

michenly merged commit 3eff97a into master Apr 25, 2019

michenly deleted the use-query branch April 25, 2019 15:25

michenly temporarily deployed to production April 25, 2019 16:16 Inactive


		### `useApolloClient`

		`useApolloClient` hook can be use to access apollo client that is currently in the context.

Conversation

michenly commented Apr 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lemonmade 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michenly Apr 25, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michenly commented Apr 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemonmade 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michenly commented Apr 18, 2019 •

edited

Loading

michenly Apr 25, 2019 •

edited

Loading

michenly commented Apr 24, 2019 •

edited

Loading