HenrikJoreteg / redux-bundler

Compose a Redux store out of smaller bundles of functionality.
https://reduxbundler.com
583 stars 46 forks source link

What is the proposed pattern to implement "item details bundle"? #50

Open abuinitski opened 5 years ago

abuinitski commented 5 years ago

Background: Main app page has a list of items that is on lower level managed by Async Resource Bundle, and on a higher by a custom bundle that controls lifecycle of this list – when it's fetched, invalidated etc.

Details page needs to also download item details, and we want to cache / expire / retry etc on individual item level – so we can navigate to one item, wait for it to download, then go to another item, and when going back to item 1 – to reuse cached version of it. Therefore we need a similar factory to createAsyncResourceBundle but which will be able to manage a collection of individual items (each item to be referenced by it's item identifier), managing their cache lifecycle independently.

Is there a better/recommended way to do this? Would a PR with such implementation be welcome?

P.S. never had a chance to thank you for redux-bundler – it is an ultimate perfect solution and a genius approach to Redux and state management. Pattern of all my frontend engineering experience life :)

HenrikJoreteg commented 5 years ago

hey @abuinitski I have, in fact done this before, just haven't shared a helper for it.

I think for that type of thing frequently the right level of abstraction is actually as set of state mutation helpers, that you just use in your reducer in your bundle that you hand-write.

for example, I frequently have a set of helpers like:

  1. getInitialState()
  2. setUpdatedState()
  3. setStartingState()
  4. setFinishedState()
  5. setFailedState()

Which do what createAsyncResourceBundle does internally.

Then you key your reducer state by id

reducer: () => {
  if (type === STARTED) {
    const existing = state[payload] || getInitialState()
    return {...state, state[payload]: getStartingState(existing)
  }
  if (type === FINISHED) {
    const  existing = state[payload.id]
    return { ...state, state[payload.id]: setFinishedState(existing)}
  }
  ...
  return state
}

P.S. glad you're enjoying it! I've been pretty happy with it on several complex projects myself.

SahidMiller commented 5 years ago

Thanks @abuinitski for asking this question and @HenrikJoreteg for answering. I've been dying to see an answer to this question!

To follow up how would one go about using createSelectors with the above solution and still take advantage of memoization at the hash item level? It's possible to not bind those selectors and just import them but I was hoping I wasn't missing something simple.

abuinitski commented 5 years ago

@SahidMiller might not answer your question if you're not using React, but I'm thinking that in general two approaches are theoretically possible:

  1. define and cleanup specific selectors/actions/reactors in runtime as specific items come and go. Logical but ugly, will complicate base framework code and will require a PR which would be very controversial to approve :)

  2. do not define any specific selectors on a bundle level, but instead create and manage them in a hook that might look somewhat like that:

export default function ProductDetails({ productId }) {
  const { product, productIsLoading, lastProductError } = useAsyncResourcesItem('products', productId, { eagerFetch: true} )
  return <span>{product.name}</span>
}

I think I'll write something like this now since I need it. It's react-specific so can't be a part of the base package – will publish as part of abuinitski/redux-bundler-hook

This example doesn't seem pretty since it somehow ties state with the fact that a component is mounted, so I'm in doubtful about eagerFetch option – might be better to add a custom reactor that would trigger a fetch based on some other bundle's selectCurrentProductId.

HenrikJoreteg commented 5 years ago

@SahidMiller @abuinitski the general question seems to be how do I pass in an ID for a specific item while still getting all the memoization / caching, etc.

Generally the specific ID of the thing you're rendering is already "selected" somehow. Most commonly it's in the URL itself. So you have something like /widget/47 as a current pathname. With redux bundler you have access to those URL params already since it tracks the current URL.

So, the easiest way is to make a selector that takes this into account, I usually do something like this:

create a route in using createRouteBundle that has a route like: /widget/:widgetId

Then you can have selectors that reference it, like this:

selectActiveWidgetId: createSelector(
  'selectRouteParams',
  params => params.widgetId
),
selectActiveWidgetRaw: createSelector(
  'selectWidgetsRaw',  // whatever selector that selects the entire reducer with individual widgets keyed by ID
  'selectActiveWidgetId',
  (widgetsReducerState, activeWidgetId) => {
    if (!activeWidgetId) {
      return null
    }
    return widgetsReducerState[activeWidgetId] ||  null
  }
),
reactShouldFetchWidget: createSelector(
  'selectActiveWidgetId',
  'selectActiveWidgetRaw',
  'selectPathname',
  'selectAppTime',
  (widgetId, activeWidgetRaw, pathname, appTime) => {
    // optionally specify this should only happen on certain pages
    if (!pathname.startsWith('/widgets/')) {
      return null
    }
    if (shouldUpdate(activeWidgetRaw, {now: appTime, staleAge: ms.minutes(5) })) {
       return { actionCreator: 'doFetchWidget', args: [widgetId] }
    }
  }
)

This way you can have a single widget reducer that stores state by widget ID like this along with the metadata about last fetches, etc.

{
  '47': {
    data: { ... }
    lastFetch: SOME_TIMESTAMP
    ... 
   }
}

Hope this helps...

abuinitski commented 5 years ago

@HenrikJoreteg actually that would really cover 95% of the cases.

My 5% is unfortunately a bit different – I not only have multiple "items", but also multiple "current item group"-s 🤣

abuinitski commented 5 years ago

Thought on a moment that it still can be covered by this approach – but on another thought – not completely since views would still have to select data relevant to them on each render.

HenrikJoreteg commented 5 years ago

@abuinitski views just "selectActiveWidgets" too. The same general approach works fine as long as you're tracking which widgets should be showing at anytime in some fashion, right? What am I missing?

abuinitski commented 5 years ago

@HenrikJoreteg sorry – you are absolutely right. Why my brain ejected this from a list of valid approaches is because each leaf component will re-render each time when any item somehow changes, instead of only being re-rendered when it's own item changes. This should not be a "real" problem, but it just somehow triggers me.

I have implemented a bundle like this and would want to publish it as a PR for redux-bundler, I'm just a bit misaligned with different timing-related cases that are present in current async bundle.

It uses "expired", "stale" and "outdated".

I assume "expired" does not bring any active effect to the bundle itself besides "isExpired" selector to become true.

Others are connected in a following way:

This all seems a bit too complicated to me. I think it might be boiled down to just two items:

So in most cases any loaded data will softly refresh itself without any visible effects to the UI, while "expiry" is a harder case – like Instagram that would show you yesterday's results while loading fresh content, but if you open it without using for a month – it will start up with a clean sheet and a loading indicator.

What do you think?

HenrikJoreteg commented 5 years ago

@abuinitski hey!

each leaf component will re-render each time when any item somehow changes, instead of only being re-rendered when it's own item changes

This actually isn't a problem at all whenever any of the input functions to the selectActiveWidgets change it will be re-run and return a new array with the relevant objects updated. So any components subscribed via selectActiveWidgets would re-render if the inputs changed because a new array would be returned.

In terms of the PR. I think I'd rather keep is an an external module, not because i don't think it's a good idea but because it feels a bit tangential. I'm actually contemplating doing the same with createAsyncResource itself. I have several projects where I'm not using it, so it feels a little unnecessary to include it in the core lib by default.

Thank you, though! I'd say just publish it as a separate npm module.

abuinitski commented 5 years ago

So any components subscribed via selectActiveWidgets would re-render if the inputs changed because a new array would be returned.

Yep, and that's what I meant originally - I would not want them to re-render. The pattern itself assumes that leaf components would actually want just one of all the items – and really don't care about the others.

Still, it doesn't change the outcome – bundle design you described earlier is clear, would work flawlessly, and rendering stuff is really a framework integration specifics that should not concern bundler at all.

I'll publish the package, thanks!

abuinitski commented 5 years ago

And voilà https://www.npmjs.com/package/redux-bundler-async-resources Hope it'll be useful for someone.

abuinitski commented 5 years ago

FYI guys,

I have extended redux-bundler-async-resources with a lot of new stuff, and re-implemented createAsyncResourceBundle there also so interface is the same, and new features are there also.

Additionally, I just finally finished a hooks package for that so both can be transparently used with react's suspense and error boundaries.

Just in case someone finds it useful.