Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] combineSlices helper with injectable slice/reducer support and selector wrapping #2776

Closed
phryneas opened this issue Oct 15, 2022 · 13 comments
Milestone

Comments

@phryneas
Copy link
Member

phryneas commented Oct 15, 2022

I'm just writing this down so I get my head emptied out and see if what I'm thinking makes any sense

reducer.ts

import { combineSlices } from '@reduxjs/toolkit'

import { sliceA } from 'fileA'
import { sliceB } from 'fileB'
import { lazySliceC } from 'fileC'
import type { lazySliceD } from 'fileD'

import { anotherReducer } from 'somewhere'

export interface LazyLoadedSlices {}

export const rootReducer = combineSlices(sliceA, sliceB, {
  another: anotherReducer,
}).withLazyLoadedSlices<LazyLoadedSlices>()
/*
 results in a return type of
 {
    [sliceA.name]: SliceAState,
    [sliceB.name]: SliceBState,
    another: AnotherState,
    [lazySliceC.name]?: SliceCState, // see fileC.ts to understand why this appears here
    [lazySliceD.name]?: SliceDState, // see fileD.ts to understand why this appears here
 }
 */

the "naive" approach

fileC.ts

import { rootReducer, RootState } from './reducer'
import { createSlice } from '@reduxjs/toolkit'

interface SliceCState {
  foo: string
}

declare module './reducer' {
  export interface LazyLoadedSlices {
    [lazySliceC.name]: SliceCState
  }
}

export const lazySliceC = createSlice({
  /* ... */
})
/**
 * Synchronously call `injectSlice` in file.
 * This will add `lazySliceC.reducer` to `rootReducer`, but **not** trigger an action.
 * `state.lazySliceC` will stay `undefined` until the next action is dispatched.
 */
rootReducer.injectSlice(lazySliceC)


// this will still error - `lazySliceC` is optional here
const naiveSelectFoo = (state: RootState) => state.lazySliceC.foo

/**
 * `injectSlice` would not inject the slice again if it is referentially equal, so it could be called 
 * in multiple files to get a `rootReducer` with types that are aware that `lazySliceC` has already 
 * been injected
 */
const selectFoo = rootReducer.injectSlice(lazySliceC).selector((state) => {
    /**
     * `lazySlice` is guaranteed to not be `undefined` here.
     * we wrap the selector and if it is called before another action has been dispatched
     * (meaning `state.lazySlice` is still `undefined`), the selector will not be called with 
     * the real `state`, but with a modified copy (or a Proxy or whatever we can do here to keep
     * it as stable as possible)
     */
  return state.lazySlice.foo
})

the next step (maybe in a later release?)

"integrated" approach - adding a selectors field to createSlice
fileD.ts

import { rootReducer } from './reducer'
import { createSlice, WithSlice } from '@reduxjs/toolkit'

interface SliceDState {
  bar: string
}

declare module './reducer' {
    // new helper `WithSlice`
  export interface LazyLoadedSlices extends WithSlice<typeof lazySliceD> {}
}

export const _lazySliceD = createSlice({
  /* ... */
  selectors: {
    uppercaseBar: sliceState => sliceState.bar.uppercase()
  }
})

// this will still error, as `state.lazySliceD` is still optional at this point
_lazySliceD.selectors.uppercaseBar(state)

// this will internally just call `rootReducer.injectSlice(_lazySliceD)
const lazySliceD = _lazySliceD.injectInto(rootReducer) 
// this will now work even if no action has been dispatched yet, as `selectors` now have been wrapped
// in a similar approach to `rootReducer.withSlice(lazySliceC).selector`
lazySliceD.selectors.uppercaseBar(state)

general interface

declare function combineSlices(...slices: Slice | Record<string, reducer>): Reducer<CombinedState> & {
    withLazyLoadedSlices<LazyLoadedSlices>() // returns same object with enhanced types - those slice states are now optional
    injectSlice(slice: Slice) // returns same object with enhanced types - that slice's state is now non-optional
    selector(selectorFn)
}

// additions to a `slice`:
injectInto(inectableReducer)

// additions to slice options:
selectors: Record<string, SelectorFn>
// this might have room for improvement - do we allow for selectors that also take other state parts into account here or do those need to be created outside? How do we allow for memoized selectors?

Open questions:

  • do we have any circular type problems anywhere here?
  • will this work nicely with nested combineSlice calls?
@markerikson markerikson added this to the 2.0 milestone Oct 15, 2022
@mq2thez
Copy link

mq2thez commented Oct 15, 2022

I rewrote all of Etsy's seller-facing webapp to be powered by similar sorts of architecture. A lot of that was based on earlier thinking/work that went into the code-splitting stuff for Twitter's early PWA, which is talked a bit about here. Our Islands architecture, written about here, took that even further out of necessity -- one redux store, any number of islands present on a page. Mostly just including this for context that I've spent 4+ years thinking about this sort of thing in one form or another.

If this had existed, it would have made a lot of our extremely-custom code completely unnecessary. You've identified and either avoided or handled the vast majority of the edge cases which were extremely tricky for us. If/when this gets released, I'd probably immediately push for us to start migrating our custom-built architecture over to using this instead. I think we would likely need the "next step" version of the selectors to fully migrate, though.

A few thoughts:

  • In our codebase, almost every selector file has a getSlice function which does exactly what your selectFoo does, and we've got additional helper code that does extremely similar selector-generation -- this is definitely a necessary dev experience thing for us. Definitely great to see you adding something like this.
  • For our types, we export SliceDState from each slice file, and that type is consumed manually by the main reducer file. It's a bit more manual, but it allows for our slices to be used in multiple reducer files -- important if you've got code-sharing of slices, or if a slice might not always be present at exactly the same spot in a tree. In this example, if we wanted to do that, I imagine we'd end up with some intermediary file that handled the declare module syntax based on knowing where in the state tree something is attached.
  • Having injection not trigger an action would work differently from how ours works now (because the current Redux interface for updating the reducer function causes an INIT action). It doesn't seem like it, but is there any risk related to memoization, dev tooling, hot reloads, etc?
  • I've never been able to come up with a way to sufficiently-generalize the injection so that we can inject anything but top-level state fields, so it would be really cool to see how you'd able to accomplish this.
  • I love the idea of a selectors field for slices, especially if it's got any amount of memoization going on under the hood, but this also feels a little bit like it's overlapping with createEntityAdapter. I think one concern I'd have would be how to have this work with more-complex createSelector cases, especially things where the cache size is >1.

@markerikson
Copy link
Collaborator

Thanks for the great feedback!

I'm generally curious how this might overlap with the much more complex "Redux modules" concepts from libraries like the ones I linked at #1326 (comment) .

I'm most definitely not saying we should have absolute full-blown "dynamic modules" support in RTK. But, I will say that it seems like a noticeable selling point for Vuex/Pinia, and that use case is a weak spot for Redux in general, so it seems worth discussing as a topic for RTK 2.0 and figuring out what is worth adding if anything.

@mq2thez could you paste in some samples of what your current setup and usage patterns look like, for reference?

@phryneas
Copy link
Member Author

* For our types, we `export SliceDState` from each slice file, and that type is consumed manually by the main `reducer` file. It's a bit more manual, but it allows for our slices to be used in multiple reducer files -- important if you've got code-sharing of slices, or if a slice might not always be present at exactly the same spot in a tree. In this example, if we wanted to do that, I imagine we'd end up with some intermediary file that handled the `declare module` syntax based on knowing where in the state tree something is attached.

This will of course also be possible. It is just a bit more circular, so if we introduce a new documented pattern here, I'd rather document both the slice and the types to be injected - but that won't mean you can just import the types to create that interface right there.

* Having injection not trigger an action would work differently from how ours works now (because the current Redux interface for updating the reducer function causes an INIT action). It doesn't _seem_ like it, but is there any risk related to memoization, dev tooling, hot reloads, etc?

That is a good point and certainly needs some investigation - but I think it shouldn't really be the case.
The general idea here is that we can inject that we do not trigger that action so a reducer can just be injected at any time - especially during a React render without interrupting it in any way.

* I've never been able to come up with a way to sufficiently-generalize the injection so that we can inject anything but top-level state fields, so it would be really cool to see how you'd able to accomplish this.

Generally this should not be too much of a problem (just nest a reducer created by combineSlices into another reducer created by combinedSlices, but the typings of the selectors might get tricky. I'll certainly have to look into that part.

* I love the idea of a `selectors` field for slices, especially if it's got any amount of memoization going on under the hood, but this also feels a little bit like it's overlapping with `createEntityAdapter`. I think one concern I'd have would be how to have this work with more-complex `createSelector` cases, especially things where the cache size is >1.

Per default, these "most simple" selectors would not even be memoized by default. That part still needs to be fleshed out and I'm pretty sure we will end up with something that will allow all three variants: "non-memoized", "memoized" and "memoized with options".

@mq2thez
Copy link

mq2thez commented Oct 25, 2022

In terms of code samples, this is a version of one of our subapps, with names altered. This sort of example lives in a Loader.ts file, which each of our "subapps" has:

// SomeSubapp/Loader.ts
import reducerRegistry from "path/to/reducerRegistry";

import reducer from "./reducer";
import OtherReducer from "../OtherSubapp/reducer";
// path to the main component
import App from "./App";

reducerRegistry.register({
    subapp1Settings: reducer,
    otherSharedCode: OtherReducer,
});

export const SomeSubapp = App;

In this example, our main Routes.ts file would be responsible for (based on the route) doing import("SomeSubapp/Loader") and extracting SomeSubapp out for react-router to render. The "Loader" file pattern ensures that every subapp has the exact same way of exposing it's top-level component while also ensuring that its reducer is lazy-loaded into state.

Most subapps define only a single field here (IE, the reducer which handles their slice of the top-level store state, subapp1Settings in this case). However, we have a number of components which might need to manage other shared state (in the above example, otherSharedCode). We do both of these in one reducerRegistry.register call because the underlying mechanism in register (through a little bit of indirection) is:

store.replaceReducer(combineReducers(newReducers))

replaceReducer triggers a run through all of the reducers, and we have one subapp in particular which pulls in a lot of different reducers (both due to legacy reasons and also due to pulling in a lot of different shared components). We discovered that having more than one replaceReducer call caused perf issues (due to running through the whole tree each time).

In a perfect world, that Loader file (because it holds the register call) would export types, but (again, due to the shared components that could be used in multiple places), that didn't quite work for us (and it also caused some circular dependency issues).

Instead, we typically define / export the relevant types in our Selector files:

import type { Action } from "redux";
import type { ThunkDispatch } from "redux-thunk";
import { useSelector, useDispatch, type TypedUseSelectorHook } from "react-redux";

import type { SubappStateType } from "path/to/subappStateTypeHelper";
import type { SomeSubappSliceType } from "./reducer";

export type SubappStateType = SubappStateType<{
    someSubapp: SomeSubappSliceType;
}>;

export type SubappDispatchType = ThunkDispatch<
    SubappStateType,
    unknown,
    Action
>;

export const useSomeSubappSelector: TypedUseSelectorHook<SubappStateType> =
    useSelector;
export const useSomeSubappDispatch: () => SubappDispatchType =
    useDispatch;

function getSlice(state: SubappStateType) {
    return state.someSubapp;
}

// various selector functions follow

In this case, SubappStateType is a helper which defines SubappStateType as having a number of fields which will always be present (some reducers/data which are not lazy-loaded), and changes someSubapp from unknown to SomeSubappSliceType.

These samples all come from our seller app -- our buyer-facing Islands code does similar stuff, though with abstractions in some different places.

@mq2thez
Copy link

mq2thez commented Oct 25, 2022

As I mentioned above, we've got home-grown utilities that mostly-solve a lot of these problems, but it's not hard to see how we could replace what we've got with what you're looking at. Our solutions have a number of limitations, such as that all "shared" components have to add new top-level state fields. It seems like your solution would remove that limitation and make a number of things more flexible, with a style that feels like hacked-on and more organized.

@airjp73
Copy link
Contributor

airjp73 commented Nov 5, 2022

Hi!

This API sounds like it could be really useful and could probably even handle the "dynamic modules" case that Mark was mentioning. I've definitely been in situations where I had a complicated micro-frontend / sub-app / widget that used redux, but in order to render multiple different versions of it and have nice DX, I opted to give each sub-app its own redux store & provider. Being able to easily inject / clean-up individual slices could enable that kind of architecture to tap directly into the app-wide store and respond to app-level events more easily.

Reading this got my gears turning, so I threw together a proof-of-concept, trying to follow Lenz's proposed API closely. I didn't implement any of the selector stuff though -- just the loading / unloading part. It looks like this could make lazy loading & dynamic modules much easier to implement.

https://codesandbox.io/s/gifted-galois-9er3id?file=/app/combineSlices/combineSlices.ts

@phryneas
Copy link
Member Author

By the way, for anyone subscribed here: @EskiMojo14 is currently working on this over in #3297

@mq2thez
Copy link

mq2thez commented Apr 18, 2023

WOOOOO just saw this pop up in 2.0.0-alpha.5!

@markerikson
Copy link
Collaborator

markerikson commented Apr 18, 2023

@mq2thez @agusterodin yeah! try it out and let us know what you think?

@mq2thez
Copy link

mq2thez commented Apr 19, 2023

It looks like we've got a few more steps worth of tech debt to eliminate before we can adopt something like this, but I can see a decently clear path.

It seems like the store.replaceReducer call is still left to us in this implementation, right? The inject API appears to return a new reducer, rather than having some kind of internal mechanism for updating the store or modifying the existing reducer function.

@EskiMojo14
Copy link
Collaborator

EskiMojo14 commented Apr 19, 2023

inject modifies the existing reducer function that's being used by your store. it also returns that same instance of the reducer, typed to now know that that slice has been added. it's similar to api.injectEndpoints.

it's worth noting one difference between inject and replaceReducer - replaceReducer immediately dispatches a "replace" action, ensuring that the state is updated by the new reducer. on the other hand, inject doesn't dispatch any actions, so injected reducers will only appear in state once an action is dispatched.

@mq2thez
Copy link

mq2thez commented Apr 19, 2023

@EskiMojo14 awesome, thank you for the clarification (and all of the hard work)! I'll add that to my notes.

At the moment, I think our biggest blocker / thing I'll have to figure out is whether immer@10 will work with polyfilled implementations of Proxy / Reflect / Symbol etc. I'm starting to put together a doc for our upgrade path and the things we'll need to investigate/resolve/etc.

@markerikson
Copy link
Collaborator

Going to close this since it's available in 2.0-beta.0, but we'd love more feedback on how it works out in practice! Very much open to tweaking the API design before any final release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants