redux-toolkit icon indicating copy to clipboard operation
redux-toolkit copied to clipboard

Future planning: RTK 2.0?

Open markerikson opened this issue 3 years ago • 26 comments

UPDATE 2022-1015: THIS IS NOW MUCH LESS HYPOTHETICAL AND HOPEFULLY HAPPENING SOON-ER THAN LATER!

~~## THIS IS HYPOTHETICAL AND WE ARE NOT GOING TO BE RELEASING RTK 2.0 ANY TIME SOON PLEASE DON'T PANIC~~

Okay, with that disclaimer out of the way: what would an RTK 2.0 look like? What "breaking" changes would we want? When should we actually consider doing that?

The most obvious ones I can think of would involve changing back-compat publishing stuff. In particular, dropping the default enableES5() plugin call for Immer, and dropping publishing IE11-compatible syntax from our published build artifacts.

Related Twitter thread:

  • https://twitter.com/acemarke/status/1376381894279987209
  • https://twitter.com/acemarke/status/1376385867699326977
  • https://twitter.com/acemarke/status/1376386257434071041

I suppose the hypothetical Redux 5.0 with potentially altered TS types might fit in there, but I truly have no idea if we're ever going to get around to releasing that.

Right now we're planning to put RTK Query out in RTK 1.6. It's additive, so it's not breaking.

The ESBuild PR in #957 is problematic for IE11 compat out of the box, but it's likely we could down-compile the ESBuild output to resolve that.

Any further ideas?

markerikson avatar Mar 29 '21 04:03 markerikson

Just curious; what would be the blocker for Redux 5.0? I apologize, I missed that discussion and thought it would be useful to include it as something to track in this discussion

crutchcorn avatar Mar 29 '21 04:03 crutchcorn

So if you look over in https://github.com/reduxjs/redux on master, the Redux core was actually converted to TS almost two years ago.

And it's just sat there ever since, unreleased.

Sorta related to that, there's some changes to the redux-thunk typings that have been sitting around unreleased for a very long time.

There's a couple different potential things here:

  • @timdorr was overseeing both of those changes, and he's been extremely busy with his day job for a while now
  • A major release of any lib is always a big deal, and there's a lot of Redux-related packages that might need to get updated to say they work with 5.x
  • For TS specifically, there's almost definitely some breaking changes in the Redux core TS types, either because something got "fixed" during the TS conversion, or something accidentally got broken

Meanwhile, Redux 4.x works just fine, both code and types.

So, basically it's about no one prioritizing the effort, combined with concern about breaking the ecosystem (and potentially for not much real benefit).

I am planning to put out a new Redux 4.1.0 release in the near-ish future that would extract our error messages to "error codes" to shave down prod size, tweak a couple error messages, and remove the remaining deps like `symbol-observable. I want to get that into RTK 1.6 too.

But at this point, we have no real plans to actually get Redux 5.0 out the door.

markerikson avatar Mar 29 '21 04:03 markerikson

So my thought is that RTK-Query would actually be a "big enough addition" to actually "sell" a 2.0, compared to a tooling change.

As for the bigger changes I'd include:

  • RTK-Query
  • entityAdapter indices
  • add ESM "modern" build targeting transpilation/bundlers
  • add ESM build targeting browsers
  • keep CJS build, pretranspiled with a target for all modern browsers
  • add a new CJS entry point .../compat for every entry point that would also target IE11 (only this build would contain enableES5)

phryneas avatar Mar 29 '21 16:03 phryneas

My inclination at the moment would be to actually do most of that in RTK 1.6 or 1.7.

Most of that is additive - either new APIs or new build artifacts. Agreed that major version numbers can also be used to indicate "big new features", but I see this as more of a parallel to React Hooks coming out in a 16.8 minor, and 17.0 being used for just compat and cleanup.

I think that doing it as a minor build would make it a bit easier for folks who are using RTK to get the new RTK Query functionality more automatically.

markerikson avatar Mar 29 '21 16:03 markerikson

Any further integration of RTK Query into Redux Toolkit would be awesome. I've been switching a lot of simple fetch/ mutation requests that were originally in slices over to it (new to React/ Redux in general so I was placing a bit too much in Redux Toolkit). My code has slimmed down quite a bit already, however occasionally I want to update a piece of global state in redux through extraReducers from an RTK Query mutation. I'm not sure if this is currently possible or even advised too if it is (can't find in the docs). Cheers!

rocon4 avatar Mar 29 '21 17:03 rocon4

@rocon4 yep, and that's why we're planning to merge the RTK Query functionality back into the RTK package itself - to have it all right there and accessible without needing to install other packages (which, in some cases, may ease corporate approval processes and such).

Usage questions probably oughta go over to a Discussion thread or Reactiflux, but off the top of my head, I think you could import the thunks that are auto-generated by createApi into your other slice files and handle those actions there as well, just as if you'd called createAsyncThunk yourself and were wanting to handle that in a slice.

markerikson avatar Mar 29 '21 17:03 markerikson

Any further integration of RTK Query into Redux Toolkit would be awesome. I've been switching a lot of simple fetch/ mutation requests that were originally in slices over to it (new to React/ Redux in general so I was placing a bit too much in Redux Toolkit). My code has slimmed down quite a bit already, however occasionally I want to update a piece of global state in redux through extraReducers from an RTK Query mutation. I'm not sure if this is currently possible or even advised too if it is (can't find in the docs). Cheers!

@rocon You for sure can. All endpoints have matcher properties on them.

extraReducers: (builder) => {
    builder
      .addMatcher(
        api.endpoints.yourEndpoint.matchFulfilled,
        (state, { payload: { result } }) => {
              // do things with the result
        }
      );
  },

Feel free to jump on reactiflux or file an issue in the rtkq repo before it gets merged in if you have any issues. It's on my list to update in the examples as more focus will be on integrating with RTK specifically in the near future.

msutkowski avatar Mar 29 '21 17:03 msutkowski

Here's my thoughts atm:

  • We just merged in the big TDX->ESBuild conversion over in #957 . The build artifacts we're generating should effectively match what we've had so far: ES5 syntax and IE11 compat for CJS/ESM/UMD, plus a new set of "modern" artifacts that target bundlers and browsers. So, in theory, none of the build changes should be breaking yet.
  • I want to get the RTKQ stuff out in a minor build to help increase the initial uptake on that. Plus, it's all additive APIs, much like React released hooks in a 16.8 minor. (I can also see an argument for only releasing those in a 2.0 as a carrot to get people to upgrade, so I'm willing to listen to debate on this point.)
  • It sounds as if including an exports field in package.json is effectively a breaking change due to Node compat concerns, so we can't do that yet
  • I'd notionally assumed that RTK 2.0 would include Redux 5.0 whenever that happened, but we still have no concrete plans to release Redux 5.0 any time soon
  • However, Tim recently mentioned to me that he might have some time to look at the Redux 5.0 work in the semi-near-ish future.

So. My plan atm is to go ahead and continue with this next release as RTK 1.6, including the build tooling changes and RTKQ. We'll keep IE11/ES5 compat for now. That minimizes changes and gives more people a chance to adopt RTKQ out of the box.

We'll hold off on a 2.0 release for now. When it's time to do that, we'll drop IE11 compat and probably the UMD builds. It would be nice if we had a Redux 5.0 release ready to go so that we can sort out all the potential TypeScript issues that are likely to occur as part of that.

markerikson avatar Apr 02 '21 15:04 markerikson

I suppose another set of questions is, what about our actual APIs would we want to drop or change that would be breaking?

I'll toss one out: the object syntax for createReducer and createSlice.extraReducers. Might be a bit controversial, especially given that it was a bit of a selling point for RTK early on, but given that we strongly emphasize the builder syntax at this point I think it's justifiable.

Any other suggestions?

markerikson avatar Apr 03 '21 00:04 markerikson

A couple others that could potentially be dropped:

  • getDefaultMiddleware export - The callback notation for the middleware property is superior in the high majority of cases given that it will be pre-typed (note that I've opened #1258 to deprecate it already)
  • unwrapResult export - Now that the dispatched thunk from cAT has an unwrap property directly, I don't think there are many use cases for using unwrapResult explicitly. However, given that it can be called on the action rather than the promise that resolves to the action, it theoretically enables alternate use-cases

Shrugsy avatar Jul 07 '21 11:07 Shrugsy

How about state persistence for offline support?

grumpyTofu avatar Jul 08 '21 03:07 grumpyTofu

@grumpyTofu Not sure what you're suggesting there. File a separate issue / discussion thread to provide details?

markerikson avatar Jul 08 '21 18:07 markerikson

@markerikson based on the previous discussion, it looks like you are looking for ideas that could be really cool and significant enough to substantiate a '2.0' release, right? RTK Query already solves caching, so what about making everything else persistent? Or maybe adding some online/offline listeners to api slices so that a query could detect if the user is offline and manage the state of whatever a user would do while offline and subsequently make necessary api calls once the connection is restored?

grumpyTofu avatar Jul 08 '21 22:07 grumpyTofu

Sort of. I actually saw a comment from someone in the Ember community the other day along the lines of "minor versions are for new functionality, majors are for removing deprecated things" - ie, a major actually shouldn't have much big new stuff. I can kinda see the point in that.

Offline stuff is its own entirely complicated area, and I don't think it's anything any of us maintainers have any experience with. That's part of why I suggested opening up a separate thread - it'd be easier to discuss what "offline support" even means, sketch out potential requirements, and figure out if it's a thing we'd even want to try doing.

markerikson avatar Jul 08 '21 22:07 markerikson

Note for later reference - Sindre Sorhus has a big gist with instructions on switching to pure ESM:

https://gist.github.com/sindresorhus/a39789f98801d908bbc7ff3ecc99d99c

markerikson avatar Aug 12 '21 15:08 markerikson

Added an issue noting that we should mark the object case reducer syntax as deprecated with a runtime warning:

https://github.com/reduxjs/redux-toolkit/issues/2301

markerikson avatar May 03 '22 10:05 markerikson

Per https://github.com/reduxjs/redux-toolkit/issues/930 , we should consider replacing the current Dictionary<T> type in the entity adapter state, which treats values as T | undefined, with a simple Record<key, T> that just says "sure, this item always exists no matter what index is being used".

markerikson avatar Aug 03 '22 17:08 markerikson

We should also revisit a lot of our TS types and see if they can be simplified now that we're dealing with more modern TS (4.2+).

Vaguely related, someone was arguing on HN that their similar implementation is simpler:

  • https://gist.github.com/baetheus/2e16ad4118b6fcd45e45756780ebb22a
  • https://news.ycombinator.com/item?id=32574612

markerikson avatar Aug 24 '22 03:08 markerikson

Dropping a key link in here about how to improve package publishing:

https://github.com/frehner/modern-guide-to-packaging-js-library

markerikson avatar Sep 01 '22 16:09 markerikson

Hypothetical idea. The types for createSlice are a pain, and part of that is the need to deal with prepare callbacks.

Asked Lenz about it and he pseudo-coded something like this as an idea to investigate:

const slice = createSlice({
   ...,
   // make this a callback
   reducers: (build) => ({
     normalReducer(state, action: PayloadAction<foo>) {
       ...
     },
     somePreparedReducer: build.preparedReducer(
       // prepare
       (a: number, b: string) => ({ payload: a, meta: b }),
       // reducer
       (state, action) => {
         ...
       }
     )
   })
})

(edited by Lenz to make it more feasible)

markerikson avatar Sep 06 '22 14:09 markerikson

Also, something to consider:

  • encourage mounting slices at slice.name, or add a new slice.reducerPath option.
  • as a consequence, add a slices option to configureStore
  • as another consequence, create a combineSlices function that combines an array of slices into a reducer (and as a second argument, also accepts a reducer map object)

This would be good groundwork to add a selectors field to createSlice. Per default (or only if slice.reducerPath is set), there could be slice.selectors.foo on the slice object. For slices mounted somewhere else, there could still be a slice.getSelectors(rootState => sliceState) function.

phryneas avatar Sep 06 '22 14:09 phryneas

A couple more Twitter requests for RTK 2.0 ideas:

  • https://twitter.com/acemarke/status/1572243884704681985
  • https://twitter.com/acemarke/status/1586385909745852423
  • https://twitter.com/acemarke/status/1481720259975344128

and a poll on Redux code-splitting usage:

  • https://twitter.com/acemarke/status/1580991161358962688

markerikson avatar Sep 20 '22 15:09 markerikson

More thoughts: see if we can optimize our TS types performance (like, time it takes TS to parse things).

Ref thread: https://twitter.com/AndaristRake/status/1574836992214536193

markerikson avatar Sep 27 '22 19:09 markerikson

TanStack Query is discussing changing their cacheTime flag (equivalent to RTKQ's keepUnusedDataFor option) over here:

https://github.com/TanStack/query/discussions/4252

might be worth considering seeing if there's a couple ways we could align our APIs for consistency.

markerikson avatar Oct 01 '22 23:10 markerikson

Check to see about turning on TS's declarationMap option for better "Go To Definition" behavior:

  • https://github.com/expo/expo/blob/837aa7ef2d0d62f796aa08555a42bd327891fc83/packages/expo-module-scripts/tsconfig.base.json#L15
  • https://twitter.com/Baconbrix/status/1577321773208670210

markerikson avatar Oct 04 '22 23:10 markerikson

Lenz is tossing around ideas for maybe having some form of selectors built into createSlice, and also maybe a combineSlices helper:

  • #2776

markerikson avatar Oct 15 '22 15:10 markerikson

Some PRs that got accidentally closed with 1.9 merging:

  • #990
  • #2420
  • #2099

markerikson avatar Nov 04 '22 14:11 markerikson

Per https://www.reddit.com/r/reactjs/comments/zxcuvq/redux_fetch_react_useeffect_best_practices/j24eh8f/?context=5 :

The most common scenario that I am facing is that I would have different screens with different representations of the same entity (say, one screen is showing one set of pokemon data, whereas another screen is showing a somewhat different set of pokemon data), for which I would fetch different sets of data from the same graphql api. Ideally, I would just have one slice for one screen, and another slice for another screen; in which case both slices could have their own getPokemon method, each of which appropriate for a given screen. Instead, I find myself using a single api slice, split across screens using the injectEndpoints approach, as a consequence of which I cannot have more than one getPokemon method. Instead, I would have to come up with unique names for these endpoints (e.g. getPokemonForScreen1, getPokemonForScreen2, etc.).

This really breaks the notion of modularity. One would expect to be able to name things however one wishes within a single module, being safe in the knowledge that names are scoped within a module, and cannot clash with anything in other modules. But the injectEndpoints approach has the unfortunate consequence of losing this name encapsulation. This gets more noticeable the larger the project grows.

My thoughts:

I might have said this over there, but my first response would be that every tool has limitations, and this seems like a use case that is at the far reaches of what RTKQ is designed to do. Lenz had some particular design goals in mind when he created RTKQ, but the community has (not surprisingly) wanted to use the lib in dozens of ways we never could have foreseen ourselves. So, the case of "multiple representations of the same item" and "multiple screens needing those different representations" is something we simply have never even thought of doing ourselves, no one has brought it up before, and so nothing in RTKQ's design is intended to make that use case simpler.

Hypothetically, a potentially simple fix would be to update defaultSerializeQueryArgs to include the reducerPath value in the serialized name, like: "pokemonApi:getPokemon('pikachu')". Alternately, the internal slice reducer could switch to storing the values nested a level deeper, like: queries: {pokemonApi: {"getPokemon('pikachu')"}}, etc.

That said, it could cause a couple issues.

We do consider the internal data structure to be intentionally "opaque", ie, it's not specifically documented and users are expected to interact with it via the selectors and hooks rather than direct access. But, the values are just right there in the Redux store and visible same as any other pieces of the Redux state. So, changing that would cause the (relatively rare) cases where users are already introspecting the state themselves to break, because the key strings would have changed.

Also, the Redux DevTools Extension has an "RTK Query" tab, and I'm assuming it derives the endpoint names the same way, by inspecting the keys. That would also probably break if we changed the key format or state structure.

Given that we're looking at RTK 2.0, something like that could be on the table as a notional breaking change, but the question is whether this relatively rare use case is enough to justify it.

markerikson avatar Dec 29 '22 16:12 markerikson

@markerikson this is pretty normal and for example Apollo Codegen has the same requirement - you can have a hundred GraphQL queries that query similar data, but each one of them should have a unique operation name.
That translates 1:1 to our endpoints.

phryneas avatar Dec 29 '22 17:12 phryneas

@phryneas : yeah, I think the complaint here is that "unique operation name" gets annoying when you have multiple conceptual representations of the same thing, ie, a "mini Pokemon" that might just be {id, name, type}, and another that might be {id, name, type, abilities}. They're both a Pokemon, so you might name both of the endpoints getPokemon and have a conceptual naming clash.

markerikson avatar Dec 29 '22 18:12 markerikson