Discussion items #15
Comments
Maybe a week, or two? Might be beneficial to wait for the TS4.1 release
Right now, the ESM build is 14kb gzipped. Not too happy with that myself, but it does a lot.
On a TS level, in RTK you can't use the middleware if the reducer is at the wrong place. I'd rather have it complain somewhere else, but that's the only external assumption you can make with Redux types. No runtime checking atm.
Right now, the structure is: Looking at this: we could get rid of the So we'd come up with something like this @msutkowski, your thoughts?
Since we're explicitly dealing with non-normalized data, that would mean doing what react-query and swr do: Have the library user update the cache with an expected next response data. While I doubt that anyone would really want to do that, we could do what those libraries do: allow the user to directly manipulate cache state with some extra actions, probably in a
So far, I've looked at
Triggered by you now, I've also taken a look at the ones you listed here:
So from scanning over the docs of these libraries, I haven't really seen any killer features we definitely need in addition to what we have at the moment. But some of them might (I think mostly redux-request & redux-resource) become more relevant with a second fetching-helper-library concerned with primarily normalized data (I really think we should keep that separated, it's just too different in use case!) |
|
Lemme give a concrete use case that I'm not sure I'm hearing we handle. In a prior project, the app let the user edit points and polylines on a globe. Basically looked and felt a lot like Google Earth. The app had a treeview on the left to show Where it got tricky was that when editing a polyline, we wanted to still show the original values in the tree, but the globe and the form needed to show the current WIP values as you're editing. For example, if I add or change a point, I want to see the line on the globe reflect that immediately. So, my solution was to copy the entire Mm... now that I describe all that, it's not quite the best use case example for the real problem use case I want to get at, but I might as well leave these paragraphs here since I just wrote them 👍 Anyway, my actual concern is that the user may want to write additional reducer logic to manipulate these items on the client side. I would assume that some updates would also need to go through the server and be saved, but it's possible the client might just want to fetch the data and then work on it more locally. How well do we handle that, if at all? My concern is that the use case switch over to just "caching fetched data" precludes actually doing "state management" with that data on the client side because the state structure gets too complex. Hmm. For that matter, if the data is all being stored in our "cache slice" anyway, it's gonna be hard to either A) transfer a copy of it to another portion of the state tree, or B) manipulate it inside the cache slice. |
|
Size-wise, a quick check of Bundlephobia shows:
So, 15K seems pretty typical here, but always nice if we can shrink that |
With the scope being "non-normalized data, potentially badly structured or unstructured apis", not at all. I really think that's a separate concern that we'd have to solve with a second lib/entrypoint to keep both as focused on their use cases as possible. |
|
This might just have solved endless scrolling though: if we add a per-endpoint On the other hand, I don't love apollo's approach. Gotta think about it :) |
|
What's the definition of MVP releasable 😄 ? Technically, it'd work just fine right now, but I wouldn't push for that. There is a lot of testing that still needs to be done before I'd comfortably recommend it. I can put together a lot of testing, docs, and 'kitchen sink'-ify the existing examples really quickly, but I'd like to focus on the core first. |
|
yeah, we don't have to have "final" docs, but I'd like to see:
|
|
FYI, this article has some good bullet points to consider: https://nosleepjavascript.com/redux-data-fetching-antipattern/
I think what we're putting together here is on the right track to answer most of those, although it seems like we've got a somewhat centralized setup that doesn't resolve the "do I need to make all my data fetching actions at the root of my app?" part. |
|
Someone just pointed me at https://amplitude.github.io/redux-query/ , which seems like it's another lib worth comparing against. Looking at it, this does seem awfully similar to what we're doing here. From the front page:
|
|
and a Twitter question thread: https://twitter.com/acemarke/status/1326344484813230081
|
|
Okay, one more other lib to eyeball: very different than the React-Query API approach we've got going on here, it's really more of a "super-CRUD-async-Entity-adapter" thing, but maybe relevant a little? |
|
More discussion questions: What about code splitting? Right now this appears to require importing all of the API objects at app setup time in order to create the middleware. That would seem to preclude a code-split feature that is lazy-loaded. I know that lazy-loading Redux features is a common-ish use case, so we need to come up with a story there. Can the middleware be updated to accept adding one of these things at runtime? Also, not sure what the latest state of the API is, but looking at the sandbox at https://codesandbox.io/s/rtk-simple-query-demo-migxd?file=/src/app/services/posts.ts , this still feels too verbose: export const postApi = createApi({
reducerPath: 'postsApi',
baseQuery: fetchBaseQuery(),
entityTypes: ['Posts'],
endpoints: (build) => ({
getPosts: build.query<PostsResponse, void>({
query: () => 'posts',
provides: (result) => result.map(({ id }) => ({ type: 'Posts', id })),
}),
addPost: build.mutation<Post, Partial<Post>>({
query(data) {
return {
url: `posts`,
method: 'POST',
body: JSON.stringify(data),
};
},
invalidates: [{ type: 'Posts' }],
}),
getPost: build.query<Post, number>({
query: (id) => `posts/${id}`,
provides: (result, id) => [{ type: 'Posts', id }],
}),
updatePost: build.mutation<Post, Partial<Post>>({
query(data) {
const { id, ...post } = data;
return {
url: `posts/${id}`,
method: 'PUT',
body: JSON.stringify(post),
};
},
invalidates: (result, { id }) => [{ type: 'Posts', id }],
}),
deletePost: build.mutation<{ success: boolean; id: number }, number>({
query(id) {
return {
url: `posts/${id}`,
method: 'DELETE',
};
},
invalidates: (_, id) => [{ type: 'Posts', id }],
}),
}),
});
|
Reading "all of the API objects", I think this is a misconception (which also explains some of your other assumptions): In general, an application should have one api object. Having multiple api objects would mean, really, querying different APIs, like one REST api and one GraphQL api. It would really be more interesting to try & "lazy-load" additional endpoints into one already-created api definition. Could be worth an experiment. It's just the question on how to type that. Our examples are just using multiple apis to keep the examples as self-contained as possible right now. This also concerns code splitting: we can (and probably will) come up with a way of post-loading apis into the store - but that would be an edge case. This is more in line with apollo that requires you to configure all your api's caching behaviour in one central place than with react-query that makes you litter your logic into just about any component.
Shouldn't be necessary any more, but I think we forgot to adjust some typing somewhere, I encountered this yesterday as well.
This goes with what I said above: assume this API returns 15 different data types.
You can also decide to skip
That could be implemented in the
That would remove the "transport type agnostic" part. Right now, whatever is returned from that |
|
When I said "API object", I meant one of those "service" files like the Say I have export const createStore = (options?: ConfigureStoreOptions['preloadedState'] | undefined) =>
configureStore({
reducer: {
[counterApi.reducerPath]: counterApi.reducer,
[postApi.reducerPath]: postApi.reducer,
[timeApi.reducerPath]: timeApi.reducer,
},
middleware: (getDefaultMiddleware) =>
getDefaultMiddleware().concat(counterApi.middleware, postApi.middleware, timeApi.middleware),
...options,
});the slice reducers could always be injected, but the need to add those middleware ahead of time seems like it makes code-splitting impossible. |
|
Yeah, that was my point: that should not be something common.
But yeah, the other case of lazy-loading completely different apis is on the list :) |
|
Looking at this latest example code: import { createApi, fetchBaseQuery } from "@rtk-incubator/rtk-query/dist";
export const api = createApi({
baseQuery: fetchBaseQuery({ baseUrl: "https://rickandmortyapi.com/api/" }),
entityTypes: ["Episode", "Character", "Location"],
endpoints: (build) => ({
getEpisodes: build.query({
query: () => "episode",
provides: (result = []) => [
...result.results.map(({ id }) => ({ type: "Episode", id })),
{ type: "Episode", id: "LIST" }
]
}),
getEpisode: build.query({
query: (id) => `episode/${id}`,
provides: (_, id) => [{ type: "Episode", id }]
}),
getCharacters: build.query({
query: () => `character`,
provides: (result) => [
...result.results.map(({ id }) => ({ type: "Character", id })),
{ type: "Character", id: "LIST" }
]
}),
getCharacter: build.query({
query: (id) => `character/${id}`,
provides: (_, id) => [{ type: "Character", id }]
}),
getLocation: build.query({
query: (id) => `location/${id}`,
provides: (_, id) => [{ type: "Location", id }]
})
})
});This is definitely looking a lot better, but I don't like the
Or maybe at least have like a yipes. this is really sounding like AngularJS Resources again... https://docs.angularjs.org/api/ngResource/service/$resource but seriously, being able to drop or generate the |
|
Poked at the "Rick and Morty" demo for a bit: https://codesandbox.io/s/ricky-and-morty-rtk-query-conversion-5gnel?file=/src/services/api.js State structure is definitely looking better. A couple questions:
Not necessarily saying that is better, just asking.
|
|
I'm sure @phryneas will give more clarity regarding some of these questions tomorrow, but I can address a few.
The first argument is the Regarding the verbosity of export const api = createApi({
baseQuery: fetchBaseQuery({ baseUrl: "https://rickandmortyapi.com/api/" }),
endpoints: (build) => ({
getEpisodes: build.query({
query: () => "episode"
}),
getEpisode: build.query({
query: (id) => `episode/${id}`
}),
getCharacters: build.query({
query: () => `character`
}),
getCharacter: build.query({
query: (id) => `character/${id}`
}),
getLocation: build.query({
query: (id) => `location/${id}`
})
})
});Regarding the usability of provides... it's grown on me after some usage. Compare it to having to do something like this wherever you perform a mutation. const [mutate, mutationState] = useMutation(patchTodo, {
onSuccess: (data) => {
// Update `todos` and the individual todo queries when this mutation succeeds
cache.invalidateQueries("todos");
},
});
They're intentional - we just stringify the args. function defaultSerializeQueryArgs(args: any, endpoint: string) {
return `${endpoint}/${JSON.stringify(args)}`;
} |
|
I'm not saying get rid of
should be automatic, and we ought to be able to leave out the |
|
Just wanted to drop this in... I didn't make any code improvements at all and kept the same behavior... but this is looking pretttttty good. https://github.com/msutkowski/rtk-query-rq-custom-hooks-conversion/pull/1/files. |
|
Just to toss a thought out there: how settled are we on the name I think it's reasonable, but I do kinda worry people will look at that and go "oh, you're ripping off React-Query". which in a way we are :) but not on a strict 1:1 basis. any other good name ideas we should consider? |
|
Well, it's about queries and mutations, so "query" is kinda close-ish. But.. what about "qumut"? Like queries & mutations? |
|
doesn't exactly roll off the tongue :) |
|
Say it three times and you'll never stop saying it because it is so fun :p Otherwise: |
|
"rtk-query" is certainly short and easy to remember. I just don't want folks to turn this into a "YOU RIPPED OFF REACT-QUERY" thing. |
|
Hm. Pretty easy to counter. "react-query is more similar to swr than rtk-query is to react-query". 🤷 |
|
heh, sure. let's stick with besides, in theory we want to merge this into RTK itself, although I was talking with @msutkowski last night about the pros and cons of doing that vs keeping it as a separate lib long-term. |
|
Yeah. I think RTK would become too big from the perspective of someone just looking at bundlephobia if we were to directly include it. Size would probably double. What I could imagine would be to split RTK up in general, like other tools started doing it:
On the other hand, that is what tree-shaking does. So no idea it it would be useful. |
|
I'm not keen on the idea of splitting up RTK. Part of the point of RTK is that it is the "all-in-one package" - add it, get all the pieces you need. And yes, RTK should tree-shake correctly, and Bundlephobia is smart enough to show that: https://bundlephobia.com/result?p=@reduxjs/toolkit@1.4.0 Mostly, anyway - I'm not convinced that all those exports each add 10.3K (like, But Bundlephobia does at least say up front that it's tree-shakeable, which is good. |
|
|
|
Oh, wow. Yeah, I was thinking that 17K was the size of RTK-Query in addition to RTK. In that case I'd say we're doing pretty well size-wise :) Would still be nice to poke into this a bit further, but we should be okay for now. |
|
@markerikson I used a bitwise operator in |
I thought so, too :D |
May I suggest:
|
I (and many of my colleagues) have experience with badly written APIs that return entities without IDs, worth having a default that assumes item.id (for simple cases) but also a way to add custom configuration (for cases where ids are not there). |
Hm. I'd dismiss |
I'm still pretty oppsed to that general idea. and these are all very sane APIs. Reality will find different ways. We either would have to provide 10 different helpers to deal with that or 3-4 helpers which are highly configurable. |
|
I see. Maybe I'm missing something, how would one fit the case when no ID is sent by the API? (of course I could pick a different unique propriety of the response as an ID, but let's say there aren't any). |
|
If you are requesting something by id, but do not get one in response, you can just use the original requested arguments for that. Just to be 100% clear here: all this is just used for invalidation & automatic refetching. If you don't care about that feature, you wouldn't have to care about entityTypes, ids, provides & invalidates at all. |
|
Great, gotcha! |
|
I'm not sure if it's a good place but I wanted to ask about the cache updates after mutations. For a context, I am looking at a possiblity to migrate from apollo where I do a lot of manual cache updates after mutations because of the nature of apollo. |
|
You can do that with https://rtk-query-docs.netlify.app/concepts/optimistic-updates - but you'll have to do that by hand. |
|
@phryneas Thanks - I totally understand that you don't want to support that option. I think what I actually need is not optimistic updates (I want to update UI after mutation response comes back from the server not before - and that's how I understand optimistic updates). However I can now see that there's So, based on your experience working with apollo, would you recommend using refetch instead manually updating cache? |
|
@apieceofbart yes, essentially you would use the tools from optimistic updates (request lifecycle handlers - onSuccess in your case & And yes, personally, I would just refetch invalidated data instead of trying to update it. |
|
I'd like to announce a new lib to make RTK Query works (with auto-generated hooks!) in Angular with NgRx. It is a 100% functional version as indicated in the RTK Query guide. Small guide, example & source code are available here: |
|
@SaulMoro that's awesome! Really neat to see people building on this idea! |
|
I'm closing everything here now. If there's any need for further discussion, that should take place in a new issue over at https://github.com/reduxjs/redux-toolkit/ |
A few random thoughts for discussion:
I realize that having predefined selectors is supposed to encapsulate that, but it seems kinda painful to deal with if you need to poke at it manually.
The text was updated successfully, but these errors were encountered: