You nailed the concept — creating a selector for each page, and then combining them into a single selector. And modifying the videosHash reducer to be similar to videosByCategory as I see you’ve done.
Sure, it’s quite a bit of code, but we have to remember the benefits: predictable state, predictable rendering, and the View layer code that results is 100% bloatless. So it makes reading component code stupid simple.
It’s very nice to encode business logic ONCE in state-related code. It means you won’t need to do this anywhere else, and can use it in many places.
When you think about it, it’s the purest way to encode the logic regarding when a page is loading. To do so completely, requires handling all the cases like you did. Anywhere else in applications from previous years where we get away with a smaller set of code to perform the isLoading check will inevitably run into some bug, sone inconsistency, some missed edge case. Period.
So though these tools result in more code, it makes it obvious what the complete set of code to 100% handle the problem is. It’s a worthy tradeoff.
Now that said, there may be another angle to solve the problem which would require less code to define what you would think is a simple isLoading boolean. Part of the challenge here is that TransitionGroup will re-render the leaving page if its isLoading prop changes as it leaves. If it was re-written so that it never passes new props to components that are leaving, then you could simply have a global isLoading reducer that is toggled on/off under simpler conditions.
See, right now, most of the extra work is about pairing the fact that something is being loaded to the page its being loaded for. If we could remove the requirement of one of these 2 booleans being related to the page, that basically shrinks the code by 75%.
That sort of solution is akin to why React has been so incredible — when you come at a problem from a different way, often you lose all the challenges innate to the previous solution.
So basically we shift the place we solve to the problem to a different area yet again. First we switched it from the View layer to the Model layer. Now we switch it back to the View layer (TransitionGroup) by making a minor tweak in how it operates, which results in both less code in the userland View layer and the userland Model layer.
I’m looking at my code now, and at first glance it looks like we can add a shouldComponentUpdate to Transition here: https://github.com/faceyspacey/transition-group/blob/master/src/Transition.js
and it will check a function you pass to it like this:
<Transition shouldUpdate={(props, nextProps => props.page === nextProps.page)} />
So the premise is that it’s allowed to update any time the props change and the page is the same, but if the page is different, it knows that it’s leaving.
I don’t have time today, but based on how much work you discovered is necessary to do this otherwise, I think it’s totally something that should be added to TransitionGroup. Maybe you wanna supply a PR?
If you did this, all you need is a simple a single isLoading selector, and none of the changes we did to those 2 reducers. The isLoading selector simply checks if videos/video exist on enter of LIST and VIDEO and if they do not, returns true and otherwise returns false. And whenever it receives VIDEOS/VIDEO_FETCHED it returns false again.
So basically this selector would do the work we added to the videosByCategory and videosHash reducers. You could then re-simplify those reducers back to what they were. And then you could remove all other selectors.
…You know, that’s still not correct. Here’s one way to make isLoading super simple: basically in the routesMap thunks we go against what I wrote about not doing a setup action, and you do a setup action if and only if the fetch for data was made. Then we can have one simple reducer that listens for this dispatch. The same dispatch is made from both the VIDEO and LIST page: dispatch({ type: ‘IS_LOADING’ }). And then it listens to both VIDEOS_FETCHED/VIDEO_FOUND to set it back to false.
So this typical solution works nicely with the fix made to TransitionGroup. And now you have way smaller code, but at the cost of an additional action.
If we could figure out how to avoid the additional action and still do this, we win the game….thinking…
Basically here’s how:
We make one reducer that has a hash of categories all with the value true. This same hash has video slugs, each, if present, with the value true:
So that’s the end of it. One extremely simple isLoading reducer that does one thing per switch case, and that one thing is roughly the same across all cases. The memory object as a default argument will always be passed, keeping it pure, without having to have a reducer that returns an object that both contains a hash and a boolean value like we were previously doing. And it’s a hash that can be applied across different routes, AND WE DON’T NEED TO BREAK OUR “GUIDELINE” OF AVOIDING A SETUP ACTION. And we dont need selectors. And we dont need to complicate other reducers. And the View layer is as simple as rendering based on a single boolean flag.
Now all we gotta do is change Transition to support a shouldUpdateprop. If and when that works, we’re in the money. We’ve already won, but if we wanted to win even more, we would make it so it doesn’t even require a shouldUpdate prop. Rather, it just knows when it’s leaving and prevents additional renders. To accomplish that requires the willLeave callback firing before shouldUpdate. If that’s the case, we can just assign this.dontReRender = true and in shouldComponentUpdate if we see that flag, return false. If we can pull that off, we’re champions. It all depends on if willLeave fires before shouldComponentUpdate . If that doesn’t work, well we have to fork ReactTransitionGroup and make it do so. That’s not out of the question whatsoever. And it’s worth pursuing so this pattern can be 1000% correct. I’m like you — I’m obsessed with correctness. In real apps, there are thousands of cases like these, and it’s these cases that destroy a codebase and make it unmanageable. Each such workaround leads to more workarounds, and more and more and more until it compounds and explodes and you quit the project or find some way to get away from it.
Lastly, it’s a “feature” that TransitionGroup updates when leaving — what if you want to change something while it’s leaving. In this case we dont need it. So ideally, it’s configurable. The default should be that no more updates are allowed once it’s leaving.
