Migrate to typescript-eslint by parkerziegler · Pull Request #239 · urql-graphql/urql

parkerziegler · 2019-05-19T22:43:51Z

This PR does a few things as a follow up to #237.

Migrates the codebase off of tslint to @typescript-eslint 🎉. This is great, because it allows us to plug into the whole ESLint ecosystem as we like. I ported over all of our existing TSLint rules using the handy roadmap provided by @typescript-eslint: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/ROADMAP.md. I also fixed our files up just a bit to match the new world order, but most changes are just switching from tslint:disable to eslint-disable.
Fixes some of our hooks dependencies to match the lint rules provided by Dan's react-hooks plugin. Our tests still pass with this change, but we'll want to doubly verify that hooks behave as expected.
Added some example tests for useQuery using react-hooks-testing-library. This more serves as a proof of concept of what tests for hooks w/out react-test-renderer and enzyme might look like. The benefits I see are:

No need to spin up our own components in tests for hooks. Instead renderHook will handle this for us.
No futzing with reassignment. react-hooks-testing-library returns us a ref containing what our hook returns, so we get a nice, easy way to access the return values of our custom hooks.
waitForNextUpdate – this API allows us to wait until useEffect calls have run and the side effect generated has completed. This feels nice because it means we don't have to call wrapper.update multiple times to see our hook values update, which feels a little hacky and arbitrary.
rerender – this API allows us to rerender our hook (or rather, the component containing our hook) and pass it new values to see how it responds. Again, this feels a bit cleaner than multiple wrapper.update calls w/ different props.

I'm not super attached to using react-hooks-testing-library, but wanted to show its API in passing tests so we can make a decision about what we like. If we want to try an enzyme upgrade to see if mount is working I'm open to it, I've just experienced pain w/ waiting on enzyme maintenance. If we want to punt on test APIs later and just merge the hooks modifications and linting I'm also totally good with that! Also @andyrichardson @kitten I know we talked about renaming the skip prop in #237, so let's continue that discussion here.

src/client.ts

kitten · 2019-05-19T22:46:30Z

src/utils/error.ts

  }: {
    networkError?: Error;
-    graphQLErrors?: Array<string | GraphQLError | Error>;
+    graphQLErrors?: graphqlError[];


This doesn't seem strictly necessary; If there's a rule for it, I'd say we should maybe disable it 🤔 It seems to be a good rule of thumb but often we really don't care about a type alias, especially when, like here, we only convert all of these and normalise them anyway

Yep, this was just a linting flag, I'll switch it back 👍

parkerziegler · 2019-05-19T22:46:55Z

src/components/Mutation.test.tsx

@@ -1,3 +1,13 @@
+import { mount } from 'enzyme';


Same here, just reordering to appease lint, but happy to reorder back 😅

parkerziegler · 2019-05-19T22:47:42Z

src/exchanges/cache.ts

@@ -1,3 +1,4 @@
+/* eslint-disable @typescript-eslint/no-use-before-define */


I actually did disable the rules in the exchanges since it would've produced a very large diff. So I guess we should have a discussion about whether we disable this rule globally or follow it globally.

I think in some cases we should just reorder it in a separate commit, so we have an easily trackable change history of what happened in these files

🆒I'll revise so that this commit just disables ESLint and we can reorder in a separate commit.

kitten

I really only have a couple of nits, but my opinion would be that we could still address them in a separate PR; this way @andyrichardson and myself could open PRs about any opinions/discussions we'd like to have on this and have those discussions separately. But since the changes are minimal right now, I don't expect that to happen a lot 😅

parkerziegler · 2019-05-19T22:52:14Z

src/hooks/useQuery.ts

+      unsubscribe.current = teardown;
    },
-    [request.key, args.skip, args.requestPolicy]
+    [request, client, args.skip, args.requestPolicy]


client and request were both flagged as needing to be included here because they're both accessed in this useCallback. client comes out of context so I think it should be included for safety. It should only ever get set once, so it wasn't affecting behavior at all previously because it never changed.

Ah, damn, ok this is a huge issue and actually introduces a bug. The request.key is the hash of the request, but we're recreating the instance on every mount on purpose, since we can't know if a DocumentNode or variables instance has changed.

Ah ok. Does that not get covered by the useMemo above, which gets recalculated whenever args.query or args.variables changes? I guess the way I see the dependencies working is:

useMemo – creates request, recalculates whenever args.query (DocumentNode) or args.variables (variables) changes.
useCallback – has a dependency on request generated by useMemo, so it should get recalculated whenever args.query or args.variables changes (which sounds like is the case for which a new hash gets generated into request.key).
useEffect – gets called whenever executeQuery (the useCallback) gets recalculated.

Just want to make sure I really grok the problem, as the hooks dependencies seem correct. Happy to change back to request.key tho!

Not really, because we don't want to rely on reference equality here. Variables are likely to be recreated by the user and forcing them to memo those can be a burden. That should be an optimisation that they can optionally apply.

The same goes for misused graphql- tag stuff, where reference equality is often guaranteed but not always

Gotcha, I'll ensure we use request.key here and we can disable the hooks lint warning.

See comment on request.key change

parkerziegler · 2019-05-19T22:53:43Z

src/hooks/useSubscription.ts

-    unsubscribe = teardown;
-  }, [request.key]);
+    unsubscribe.current = teardown;
+  }, [request, handler, client]);


Same here, both client and handler are accessed in this useCallback and it makes sense to me that they're included.

parkerziegler · 2019-05-22T06:03:54Z

@kitten @andyrichardson cleaned this up a bit more and added a few more tests cases so let me know how you feel about things! The remaining discussion items seem to be:

Are we happy w/ the hooks changes? Any other 🐛s I'm not seeing?
What do we want skip to be renamed to? block as @matheus-rosin mentioned seems good to me!
How do we like the react-hooks-testing-library tests? Should we keep them alongside the old tests? Pick one vs. the other?

Happy to make changes addressing the above here or in a follow up!

kitten

All seems well 👌 I just wish we could make request.key work without adding the magic eslint comment, but we can revisit that another time 😅

kitten · 2019-05-22T09:46:53Z

@parkerziegler I think we could rename it to pause which is also a more neutral term but does fit with the idea of "pausing" any operations being sent or updating state.

I think the tests are 👌. It's definitely more error prone to work with "act" or try to make updates behave like they usually would. Let's see how it works out. We can always delete the old tests if they start failing and are falling behind.

kitten changed the title ~~Migrate to typescript-eslint.~~ Migrate to typescript-eslint May 19, 2019

parkerziegler commented May 19, 2019

View reviewed changes

src/client.ts Outdated Show resolved Hide resolved

kitten reviewed May 19, 2019

View reviewed changes

parkerziegler commented May 19, 2019

View reviewed changes

kitten previously approved these changes May 19, 2019

View reviewed changes

parkerziegler commented May 19, 2019

View reviewed changes

parkerziegler requested a review from andyrichardson May 22, 2019 05:23

parkerziegler force-pushed the task/migrate-to-typescript-eslint branch from bd254e5 to 4dd84af Compare May 22, 2019 05:56

Migrate to typescript-eslint. Refactor hooks dependencies.

caedc75

parkerziegler force-pushed the task/migrate-to-typescript-eslint branch from 4dd84af to caedc75 Compare May 22, 2019 05:59

kitten approved these changes May 22, 2019

View reviewed changes

parkerziegler mentioned this pull request May 23, 2019

Add support for skip to Query and useQuery. #237

Merged

Rename skip to pause.

d176e51

parkerziegler merged commit d87c4f0 into master May 23, 2019

parkerziegler deleted the task/migrate-to-typescript-eslint branch May 23, 2019 23:26

		@@ -1,3 +1,4 @@
		/* eslint-disable @typescript-eslint/no-use-before-define */

Conversation

parkerziegler commented May 19, 2019

Uh oh!

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!

parkerziegler May 19, 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!

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

parkerziegler May 19, 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!

parkerziegler commented May 22, 2019

Uh oh!

kitten left a comment

Choose a reason for hiding this comment

Uh oh!

kitten commented May 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

parkerziegler May 19, 2019 •

edited

Loading

parkerziegler May 19, 2019 •

edited

Loading