Hooks API and separate packages #129

ryanashcraft · 2019-06-07T00:03:30Z

Hooks

~~Note: depends on hopefully soon-to-be released of react-redux.~~

Compatibility with react-redux v7, which uses React's new context API
useRequest: which is similar to connectRequest but only for a single query. It returns the isPending state and a refresh (aka forceRequest) callback. This hook will dispatch a requestAsync redux action when the hook "mounts" and whenever the query config's query key changes.
useMutation, which returns an isPending and mutate callback.
connectRequest has been rewritten to use hooks and is hopefully compatible with React's concurrent mode

Separate packages

redux-query, redux-query-react, and redux-query-interface-superagent. This enables users that don't use react or superagent to no longer have to depend on those libraries.
The redux-query/advanced entry point has been removed. The "advanced" middleware is now just the primary middleware.

Smaller things

No longer require update fields in query configs
Updating some dependencies
More tests for the React API

To do

Update README
Write transition docs
Dogfood/test with Amplitude on production
Publish as v3.0

To be considered

Returning more query metadata (i.e. response status) in addition to isPending for useRequest and useMutation.
Returning response metadata (like the promise return value from dispatching mutateAsync, including response body and entities) for useMutation.
Flow types for hooks and query configs.
Updated docs that go into more depth the project's principles and best practices.
Address Superagent tries to mutate frozen store on timeout #121 (Superagent tries to mutate frozen store on timeout)

trashstack

Basically just questions

babel.config.js

packages/redux-query-react/src/components/connect-request.js

trashstack · 2019-06-07T20:15:15Z

packages/redux-query-react/src/components/connect-request.js

+  return { cancelKeys, requestActions };
+};
+
+const useMemoizedActions = (mapPropsToConfigs, props, callback) => {


This custom hook feels like an implementation detail of the useEffect hook in useMultiRequest.

I'd like to spend a bit of time looking at this case.

trashstack · 2019-06-07T20:20:50Z

packages/redux-query-react/src/components/connect-request.js

+    const promise = reduxDispatch(requestAsync(action));
+
+    if (promise) {
+      pendingRequests.current.add(getQueryKey(action));


So this is the awkward thing to have in the dependency list if you were using useCallback instead here.

Yeah no idea how to do this without using refs. Don't think it's a problem do you?

No it's not. I was just trying to understand the need for the custom hook.

trashstack · 2019-06-07T20:43:05Z

packages/redux-query-react/src/components/connect-request.js

+    return () => {
+      [...pendingRequests.current].forEach(dispatchCancelToRedux);
+    };
+  }, [dispatchCancelToRedux, dispatchRequestToRedux, requestReduxActions]);


As discussed I'm not sure it's worth it just to take advantage of the built in dependency comparison.

I did do some research and this is similar to how urql and react-apollo-hooks implement their hooks. They memoize the request object or configuration based on the graphql key and then use in a useEffect dependency.

urql: https://github.com/FormidableLabs/urql/blob/master/src/hooks/useRequest.ts
react-apollo-hooks: https://github.com/trojanowski/react-apollo-hooks/blob/e590e8f21f1ae4871d641d9681f7932433c8daca/src/useQuery.ts#L126

Also I've been thinking about this more I'm finding a hard time thinking how inlining the caching would make the code easier to read. Do you mind elaborating on your feelings here?

I did do some research and this is similar to how urql and react-apollo-hooks implement their hooks. They memoize the request object or configuration based on the graphql key and then use in a useEffect dependency.

It makes me a lot happier that other people are doing it. I'll take a look at those examples.

Also I've been thinking about this more I'm finding a hard time thinking how inlining the caching would make the code easier to read. Do you mind elaborating on your feelings here?

I think that understanding referential changes (and therefore when hooks are running) is going to be one of the core difficulties devs have with hooks. When you have a custom hook like useConstCallback that does something different from standard examples it makes that problem more complicated.

I guess what I was trying to do was come up with something that was less "hooky" and maybe more familiar.

I see the memoization in those examples but they're using all the standard patterns - mostly just useMemo.

They don't seem to have needed to come up with anything like useConstCallback or used refs in that way.

trashstack · 2019-06-07T20:51:12Z

packages/redux-query-react/src/hooks/use-mutation.js

+  const mutate = React.useCallback(() => {
+    dispatchMutationToRedux({
+      ...requestReduxAction,
+      force: true,


Could you pull this up into transformQueryConfigToAction and drop this useCallback?

There still needs to be a useCallback though regardless, right?

Yes, but it should only need to be the one on line 29.

You still need a callback that is bound to the current query config though.

This is what I mean: https://github.com/amplitude/redux-query/compare/v3.0...v3.0-will?expand=1

trashstack · 2019-06-07T21:16:06Z

packages/redux-query-react/src/hooks/use-request.js

+  const transformQueryConfigToAction = useConstCallback(queryConfig => {
+    return {
+      ...queryConfig,
+      unstable_preDispatchCallback: finishedCallback,


I don't remember the significance of this. Something to do with trying to keep the props of the component sane with updates coming from different sources?

Does it still work and make sense in a hooks/concurrent world?

It's to avoid sending cancelation requests in cases when the query was just finished.

If you don't use a sync callback and instead use promises, the then callback will trigger as a micro-task after the redux store updated. If React re-renders the component/hook between those two events and the query keys change, then the component/hook's internal state will still indicate that the query is still in-flight and therefore dispatch out a cancelation action.

I'm not confident how hooks or concurrent could impact this. My guess is it may change the race to make this not as much of an issue as React might be more likely to delay re-rendering. Given that, I think it's best to keep the callback as-is.

Tangential question – I wonder if we should make the "Trying to cancel a request that is not in flight" warning dev-only (or completely drop it?). It's not such a bad thing to dispatch a cancelation action, right?

Given that, I think it's best to keep the callback as-is.

👍

Tangential question – I wonder if we should make the "Trying to cancel a request that is not in flight" warning dev-only (or completely drop it?). It's not such a bad thing to dispatch a cancelation action, right?

Yea, that sounds fine.

trashstack

Based on those answers lgtm

ryanashcraft · 2019-06-11T00:39:25Z

@ManThursday updates from today:

Only set babel node target for test env (with comment), per your comment
Simplify connectRequest diffing, as you indicated
Remove several lodash libs
Fix useMutation to actually return the mutate promise
Start flow types: hooks and query configs
Remove networkHandler from redux state and actions

New to dos:

Add flow types for getQueryKey, all redux actions, connectRequest, entities reducer

trashstack

New changes lgtm

Would be nice to refine some of the anys but presumably that's not possible for most of them.

karevn · 2019-06-12T18:11:20Z

What about Typescript types? TS is seemingly taking over these days, so even Facebook has rewritten some of their code to TS.

karevn · 2019-06-12T18:15:32Z

packages/redux-query/src/lib/update.js

+};
+
+export const optimisticUpdateEntities = (
+  optimisticUpdate: OptimisticUpdate,


I think it's better to have these ones written the same way as the ones here: https://github.com/amplitude/redux-query/pull/129/files#diff-fe52475398e3e004625a25aaf3263de2R6 (with defaults)

karevn · 2019-06-12T18:22:30Z

packages/redux-query/src/middleware/query.js

+  return pendingQueries;
+};
+
+const pick = (source: { [key: string]: any }, keys: Array<string>): { [key: string]: any } => {


The general strategy in this project is to use lodash functions (citing Ryan). Why is this custom implementation here? I believe there should be a consistency - if Lodash is a preferrable lib - then it should be used, and if custom implementations are required - why not to get rid of Lodash alltogether (which I believe is a good idea, as it would save about 30% of lib size).

As I've migrated everything to flow I have reversed my decision on that. Getting flow to work well with other libraries is a pain (unless it's something like idx that just works automatically).

My thinking on this has also evolved quite a bit. I think your original point about bundle size is valid, and generally shedding dependencies, if not really necessary, is a good thing.

karevn · 2019-06-12T18:24:20Z

packages/redux-query/src/middleware/query.js

 };

-const isStatusOK = status => status >= 200 && status < 300;
+const isStatusOk = (status: ?Status): boolean =>
+  status !== null && status !== undefined && status >= 200 && status < 300;


undefined check is not necessary here, as undefined < 200 === false && undefined > 300 === false

Flow is picky here and I prefer the explicitness. Although reading it now I think I'd prefer to write it like typeof status === 'number' &&

karevn · 2019-06-12T18:28:19Z

packages/redux-query/src/middleware/query.js

    let returnValue;
-    const { getQueryKey, ...config } = { ...defaultConfig, ...customConfig };
+    const config = { ...defaultConfig, ...customConfig };
+
    switch (action.type) {


This code would be much more readable if every switch branch had been extracted to a separate function.

Agreed! I think it's ok for now but after this is merged and you'd like to take a stab at refactoring it, I think that'd be a good change.

Sure thing - I'd like :)

ryanashcraft · 2019-06-12T19:12:19Z

@karevn We use flow at Amplitude and although it's not the popular choice today, we have no plans to move. That's why my main focus was flow, but I don't see any reason why we can't support both. Would you be up for contributing typescript types?

karevn · 2019-06-17T19:19:04Z

OK, I'll start writing Typescript typings to publish them via DefinitelyTyped then.

karevn · 2019-06-23T09:34:28Z

Btw, what about including .d.ts file into redux-query packages? I think it would be much more transparent for Typescript users.

Also, I've realised that top-level package.json contains a reference to ./bin/build.sh, which is not included into the repo.

ryanashcraft · 2019-06-24T16:08:41Z

@karevn Sounds good as long as it's relatively straightforward to maintain – key thing is to keep the flow types and typescript types in sync as much as possible. Also good catch with the build script.

ryanashcraft · 2019-06-24T17:46:23Z

@karevn Let's aim for Typescript in a later release though as 3.0 is getting really close.

karevn · 2019-06-24T19:35:19Z

@ryanashcraft yes, that's exactly the way I see it - 3.0, then TS typings. Not to make a PR to PR :)

trashstack · 2019-06-11T21:05:18Z

packages/redux-query/src/middleware/query.js

+  return pendingQueries;
+};
+
+const pick = (source: { [key: string]: any }, keys: Array<string>): { [key: string]: any } => {


Probably doesn't mean much with the current usage but could this be generic'd?

Do you like this better? Unfortunately $Shape doesn't do what I want here (not sure why) – you can still reference any fields as if they are not optional.

const pick = <T: { [key: string]: mixed }>(source: T, keysToPick: Array<string>): $Shape<T> => { const picked = { ...source }; const keysToPickSet = new Set(...keysToPick); const keysToDelete = Object.keys(source).filter(key => !keysToPickSet.has(key)); for (const key of keysToDelete) { if (picked.hasOwnProperty(key)) { delete picked[key]; } } return picked; };

Take a look at this facebook/flow#7566

I also thought $Shape worked like you did.

Oh cool. Nice find. But still strange and a little disappointing that's how you do it and not $Shape.

trashstack · 2019-06-11T21:06:39Z

packages/redux-query-react/src/components/connect-request.js

  return (Array.isArray(maybe) ? maybe : [maybe]).filter(Boolean);
 };

-const difference = (a, b) => {
+const difference = <T>(a: Array<T>, b: Array<T>): Array<T> => {


$ReadOnlyArray<T>

trashstack · 2019-06-25T01:34:30Z

packages/redux-query-react/test/hooks/use-memoized-action.test.js

+    const { result, rerender } = renderHook(() => useMemoizedQueryConfig(queryConfig, transform));
+    const returnValue1 = result.current;
+
+    // Rerending but no props chnaged and the query config is the exact same, so the return value


ryanashcraft · 2019-06-25T17:46:13Z

@ManThursday Updated. PTAL. Thanks.

trashstack

lgtm

Ryan Ashcraft added 2 commits June 3, 2019 09:39

Start 3.0 refactor, separate modules into separate packages

4d0fd7c

Make network interface required param to middleware

c2d84ce

trashstack suggested changes Jun 7, 2019

View reviewed changes

trashstack approved these changes Jun 10, 2019

View reviewed changes

ryanashcraft force-pushed the v3.0 branch from 5ede009 to 392ee16 Compare June 10, 2019 18:34

trashstack approved these changes Jun 11, 2019

View reviewed changes

ryanashcraft force-pushed the v3.0 branch from 6cc5e7f to d294540 Compare June 11, 2019 20:22

karevn reviewed Jun 12, 2019

View reviewed changes

ryanashcraft force-pushed the v3.0 branch 4 times, most recently from ec7f0ee to 3df6884 Compare June 21, 2019 02:39

Ryan Ashcraft added 5 commits June 20, 2019 19:58

useRequest hook

2cac16c

useMutation, more tests and fixes

a5860e4

Rewrite connectRequest with hooks

6b78b6f

Build config fixes

486f347

Make build script executable

80d28ab

Ryan Ashcraft added 8 commits June 21, 2019 07:46

Add top-level types.flow entrypoint

795e474

v3.0.0-rc.3

627b408

Fix careless mistake in READMEs

74e7019

Use standard .js.flow extension for types

ede5fee

v3.0.0-rc.4

ddf9592

Add more links in docs

e26f31c

Configure algolia search

014ccb4

"React hooks"

735a017

Fix build script

0d4d4d9

Ryan Ashcraft added 3 commits June 24, 2019 10:31

Make useMutation take a callback instead of a query config

2172986

v3.0.0-rc.5

36d628f

Update simple example

2127725

Ryan Ashcraft added 3 commits June 24, 2019 15:02

Fix useMutation example bugs

6af8d02

More code comments and code cleanup

c0e1730

Improved useMutation docs

34bdcef

trashstack suggested changes Jun 25, 2019

View reviewed changes

Ryan Ashcraft added 2 commits June 25, 2019 10:44

Improved flow types

7a74c26

Fix typo

4c63597

trashstack approved these changes Jun 25, 2019

View reviewed changes

Ryan Ashcraft added 5 commits June 25, 2019 11:57

v3.0.0-rc.6

9cb18c8

Fix pick implementation

26bf653

v3.0.0-rc.7

80c9d65

Add transition note about forwardRef

dad49df

v3.0.0

342a27a

ryanashcraft merged commit 3a89acd into master Jun 25, 2019

Hooks API and separate packages #129

Hooks API and separate packages #129

Conversation

ryanashcraft commented Jun 7, 2019 • edited

Hooks

Separate packages

Smaller things

To do

To be considered

trashstack 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trashstack left a comment

Choose a reason for hiding this comment

ryanashcraft commented Jun 11, 2019

trashstack left a comment

Choose a reason for hiding this comment

karevn commented Jun 12, 2019

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

karevn Jun 12, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanashcraft commented Jun 12, 2019

karevn commented Jun 17, 2019

karevn commented Jun 23, 2019

ryanashcraft commented Jun 24, 2019

ryanashcraft commented Jun 24, 2019

karevn commented Jun 24, 2019

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

ryanashcraft commented Jun 25, 2019

trashstack left a comment

Choose a reason for hiding this comment

ryanashcraft commented Jun 7, 2019 •

edited

karevn Jun 12, 2019 •

edited