Add support for skip to Query and useQuery. #237

parkerziegler · 2019-05-11T16:54:54Z

This PR picks up on the changes in #229 and would close issue #228. @kitten I read through your comments in #229 and attempted to address them in this PR. I also added a few tests to ensure things are behaving as expected, but definitely would appreciate both of your 👀 in verifying that this behaves as expected.

kitten

Cheers for picking this up, Parker 🙏

kitten · 2019-05-11T16:56:11Z

src/components/Query.tsx

@@ -58,7 +63,11 @@ class QueryHandler extends Component<QueryHandlerProps, QueryHandlerState> {
    this.executeQuery();
  }

-  componentDidUpdate() {
+  componentDidUpdate(prevProps: QueryHandlerProps) {
+    if (this.props.skip && prevProps.skip !== this.props.skip) {


If props.skip is true here, we'll never reach this.unsubscribe above 🤔

Ah gotcha. So we'd hit a case where we have a current query subscription, skip updates to true, this block gets hit and this.unsubscribe never gets called. What if we just call this.unsubscribe() before the early return here?

Yea, that'd be equivalent but it should always be preferred to just call execute again since it always unsubscribes and resubscribes if necessary, so that might make things easier on terms of it always aligning the subscription with the props

Agreed, good point. Let me do some refactoring and re-push.

parkerziegler · 2019-05-11T16:58:59Z

src/components/Query.tsx

@@ -28,6 +29,10 @@ class QueryHandler extends Component<QueryHandlerProps, QueryHandlerState> {
  executeQuery = (opts?: Partial<OperationContext>) => {
    this.unsubscribe();

+    if (this.props.skip) {


Per your suggestions @kitten moved this after the call to unsubscribe. Moreover, this would return before fetching ever gets set to true, so I don't think we need to handle resetting fetching to false.

parkerziegler · 2019-05-11T17:00:29Z

src/hooks/useQuery.test.tsx

@@ -158,7 +164,21 @@ describe('on change', () => {
 describe('execute query', () => {
  it('triggers query execution', () => {
    renderer.create(<QueryUser {...props} />);
-    act(() => execute());
+    act(() => execute!());


Non-nullary operator for tests, since execute can, from a type perspective, be undefined in the course of running these tests.

parkerziegler · 2019-05-12T16:58:29Z

@kitten @andyrichardson refactored this just now per @kitten's suggestions, let me know what the next steps you think we should take are 😄 Thanks for the review thus far 💯

parkerziegler · 2019-05-12T17:00:48Z

src/components/Query.tsx

-    );
+    let teardown = noop;
+
+    if (!this.props.skip) {


Moving the check here ensures that both this.unsubscribe will get executed and redefined in preparation for the next query. No more futzing with componentDidUpdate 😅

parkerziegler · 2019-05-12T17:02:56Z

src/hooks/useQuery.ts

+      if (!args.skip) {
+        [teardown] = pipe(
+          client.executeQuery(request, {
+            requestPolicy: args.requestPolicy,


Looking at this now, shouldn't args.requestPolicy also be included in the dependency array for this useCallback? Otherwise we might not recompute it if it changes.

Yup I believe so. We're currently allowing users to specify the request policy on a manual query execution but I believe this goes against the way hooks work.

If the request policy changes, that should be reflected in the components state and flow all the way down to the useQuery hook.

parkerziegler · 2019-05-15T19:28:59Z

src/hooks/useQuery.ts

  );

  useEffect(() => {
    executeQuery();
    return unsubscribe;
-  }, [request.key]);
+  }, [request.key, args.skip]);


Last question here for both of you @andyrichardson and @kitten -> should executeQuery be included in the dependency array here? The Hooks FAQ make a big deal about it being unsafe to exclude functions that reference props / state from the dependency array of useEffect calls. So basically this dep array would become just [executeQuery]. I'm also starting to think we might want to useRef unsubscribe. Thoughts?

Hooks FAQ on functions as dependencies: https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies

Sorry, didn't get to this 😱 Including it into the array here will probably mean that requestPolicy can change at any time which is generally good 👍

🆒 that'll come up in the next PR, gonna merge this as is 😄

@parkerziegler - yeah I think that is necessary 👍

parkerziegler · 2019-05-18T05:08:41Z

@kitten @andyrichardson how do you feel about merging this? I have some follow up changes that 1) integrate typescript-eslint just for the React hooks rules, 2) fix some dependency array issues flagged by the ESLint plugin, and 3) show what a test suite for useQuery would look like with react-hooks-testing-library. Would love to get those changes up for review soon!

andyrichardson · 2019-05-19T10:28:02Z

PR looks strong 👍👍👍

@parkerzielger minor suggestion about the wording, maybe wording could be more specific than 'skip'? I'm thinking 'autoFetch' or 'fetchOnMount'. Not a big deal either way - just my 2 cents.

Is 'cacheOnly' the same as skip? In theory I would expect cacheOnly to not fetch on mount but still fetch when calling the refetch function. I had this problem with Tipple and settled on allowing both options despite the redundancy.

Also, +1 on migrating to eslint. The react linting rules suck at times (like if you want an effect to be called exclusively on mount - this would be a linting error) and adding a bunch of ignores to valid use cases is a pain but that's better than missing a required dependency.

kitten · 2019-05-19T11:58:11Z

@andyrichardson I think auto fetch or fetch on mount could imply that we encourage hooks to remain passive on mount which is regarded as an antipattern by the React team.

For now this naming was suggested because it's also what Apollo uses I suppose, and it nicely matches up with the existing skip directive. But yea, not a fan.

It's not the same as cache Only as it'll stop to fetch at all. So it is useful to "freeze" the directives logic completely.

andyrichardson · 2019-05-20T07:55:24Z

@kitten what does "passive on mount" mean? Do you have a link to the react docs about the antipattern - I need to read up on this 📖

If in getting this right, you're proposing that manual refetch isn't possible at all when using cache only? If we're going this route we'll need to update our types for useQuery

matheus-rosin · 2019-05-20T11:45:28Z

About the word "skip", when I suggested it, I didn't even wonder if that is the most plausible 😄 (yeah, it was based on Apollo already available nomenclature). So, if I could suggest a better word (and more JS-ish), it would be prevent or even block. I rather the first one, but the second seems decent too.

good-idea · 2019-05-23T21:12:05Z

Any idea on when this update will be published to NPM?

parkerziegler · 2019-05-23T21:49:55Z

@good-idea #239 will have this, which should get merged today. I'd guess we'd be able to release it next week. I'll follow up w/ Phil there and we'll try to ship it soon!

Add support for skip to Query and useQuery.

8f8907a

parkerziegler requested review from kitten and andyrichardson May 11, 2019 16:54

kitten reviewed May 11, 2019

View reviewed changes

parkerziegler commented May 11, 2019

View reviewed changes

parkerziegler added 2 commits May 11, 2019 10:52

Unsubscribe on componentDidUpdate when skip is true.

ed6cffa

Always run unsubscribe setup and teardown.

2f6e57e

parkerziegler commented May 12, 2019

View reviewed changes

parkerziegler added 2 commits May 14, 2019 08:58

Add skip to api.md.

0c38be6

Add args.requestPolicy to useCallback dependency array.

7214b59

parkerziegler commented May 15, 2019

View reviewed changes

kitten approved these changes May 18, 2019

View reviewed changes

parkerziegler merged commit a255b46 into master May 19, 2019

parkerziegler deleted the task/228-add-skip branch May 19, 2019 01:39

parkerziegler mentioned this pull request May 19, 2019

Migrate to typescript-eslint #239

Merged

This was referenced May 24, 2019

Skip request #228

Closed

Adding 'skip' prop to query and mutation components and hooks. #229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for skip to Query and useQuery. #237

Add support for skip to Query and useQuery. #237

parkerziegler commented May 11, 2019

kitten left a comment

kitten May 11, 2019

parkerziegler May 11, 2019

kitten May 12, 2019

parkerziegler May 12, 2019

parkerziegler May 11, 2019

parkerziegler May 11, 2019

parkerziegler commented May 12, 2019

parkerziegler May 12, 2019

parkerziegler May 12, 2019

andyrichardson May 15, 2019 •

edited

Loading

parkerziegler May 15, 2019

parkerziegler May 15, 2019

parkerziegler May 15, 2019 •

edited

Loading

kitten May 18, 2019

parkerziegler May 19, 2019

andyrichardson May 19, 2019

parkerziegler commented May 18, 2019

andyrichardson commented May 19, 2019

kitten commented May 19, 2019

andyrichardson commented May 20, 2019 •

edited

Loading

matheus-rosin commented May 20, 2019

good-idea commented May 23, 2019

parkerziegler commented May 23, 2019

Add support for skip to Query and useQuery. #237

Add support for skip to Query and useQuery. #237

Conversation

parkerziegler commented May 11, 2019

kitten left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler commented May 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyrichardson May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler May 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkerziegler commented May 18, 2019

andyrichardson commented May 19, 2019

kitten commented May 19, 2019

andyrichardson commented May 20, 2019 • edited Loading

matheus-rosin commented May 20, 2019

good-idea commented May 23, 2019

parkerziegler commented May 23, 2019

andyrichardson May 15, 2019 •

edited

Loading

parkerziegler May 15, 2019 •

edited

Loading

andyrichardson commented May 20, 2019 •

edited

Loading