[WIP] manage location through MatchProvider

[pshrmn]

This is a way to manage location changes that is resistant to shouldComponentUpdate.

The main change is that <MatchProvider> components are kept updated about the current location. All <Match> and <Miss> children of a <MatchProvider> provide it with update functions. The <Match> update functions return a boolean indicating whether they match against the current location. The <Miss> update function is the same as the current one.

<Match> maintains a match state, which is the result of matchPattern and is set whenever the component updates naturally (through componentWillReceiveProps) or when its update function is called.

This passes the failing miss test that @alisd23 wrote in #4047, but may need additional tests to verify its correctness.

Related issue: #4035

[alisd23]

Looks pretty good, just a few things.

Also wondering if we should add in LocationSubscriber to the miss component, as mentioned here

Also a case we still need to consider:

• When a miss component has <Match>'s (or <Miss>'s) inside of it, currently they never update. Not sure how common this case is, but the solution is just to change <Miss> to render in a similar way to Match - wrapped by a <LocationSubscriber> and a <MatchContext> element.

[alisd23]

Rename test to something descriptive now it's passing

[alisd23]

Maybe rename this to provider now it's dealing with misses and matches? Also makes it clear where this context originates from in child components.

[pshrmn]

I'll hold off on changing this, but I agree that match is ambiguous and a different name might be preferable.

[alisd23]

Is there a reason for holding off? I feel like the Miss component accessing the match context and calling addMiss on it could be confusing.

[pshrmn]

There wasn't a compelling reason. I just pushed a change to rename match to provider. It can always be renamed again if something else is preferable.

[alisd23]

Small thing - the convention seems to be no leading space in if parentheses

[pshrmn]

@alisd23 With this, the <Miss> component will not need to render a <LocationSubscriber>. It has access to the current location through the context and will always be updated to location changes through the <MatchProvider>.

As far as testing for a <Match> within a <Miss>, I believe that that would be considered undefined behavior. #4016 (comment)

[pshrmn]

This actually breaks server rendering in its current form because that only looks at the number of elements in the matches array of a <MatchProvider>. The current test for that ('renders misses on second pass with server render context result') just doesn't fail because there are no <Match> components to be added to the matches array. I'll update that to be breaking so that that issue has to be addressed.

[alisd23]

It looks like server-rendering does work at the moment, that test is correct. The matches in the current version is an array of objects for matching <Match>'s, whereas you're making it an array of functions, 1 from <Match> component?

Did you mean this now breaks with the changes in this PR, or that it was broken originally?

[pshrmn]

If you add the 'doesn't render misses on first pass' test to the current v4 branch, it should fail.

If you look at the render function of <Miss>, it determines if it should render by calling the missedAtIndex function. The current missedAtIndex code just checks if the current item (determined by serverRouterIndex) has matches in the matchesByIdentity array and if there is a <Miss> component for it.

The second check will always be true because the <Miss> will have registered itself. The first check will be true (aka there are no matches) if no <Match> components match prior to the <Miss> component rendering. If there is <Match> that matches the current location, but it is rendered after a <Miss>, that <Miss> will be rendered on the first pass.

[pshrmn]

I had to pretty heavily modify the createServerRenderContext.js code. To be honest, I don't think it worked before, it just passed the tests.

1. <Miss> components would render on the first pass as long as there weren't any prior <Match> components that matched.
2. There was a flushed variable that was used to determine some functions when the context object is being created, but it was always false at that point. It is set to true when getResult is called, but then it wasn't actually used to do anything.

Some additional tests might be needed to verify that this is working correctly.

[pshrmn]

Am I correct here in believing that the order in which <MatchProvider>s will be rendered is always the same for the first and second pass?

[alisd23]

Because registerMatchContext is called in componentWillMount, and (I think) componentWillMount is called in sequential order in the children - it is determinstic.

But also: what is the reason for flushedIndex? Doesn't seem to be used anywhere apart from the incrementing.

[pshrmn]

On the first pass, each <MatchProvider> adds an item to the matchContexts arrays. This is done through the registerMatchProvider function, which returns the index of the added item. That index is stored by the <MatchProvider> as serverRouterIndex. While the first pass is rendering, it will use the index to manipulate the item in order to indicate if it a) has any matches and b) has any <Miss> components.

On the second pass, we want to reference the items in the array that were created during the first pass. For each <MatchProvider>, we receive the index for the correct item in the matchContexts array via the call to registerMatchProvider. This is done using flushedIndex variable, simply incrementing the index returned on each call (that's why the render order is important).

[alisd23]

Ok I see, so you're just replacing the array add behaviour (which returns an incrementing index each time its called) with an incrementing integer variable. Makes sense.

[pshrmn]

I'm not entirely sure what the idea with these ternary statements are. They execute when createServerRenderContext is called, at which point flushed === false, so they are only ever the second option.

[alisd23]

I think it was probably there because the second render pass might still trigger that setRedirect function, so it is actually possible to trigger k (although you're unlikely to see a chain of redirects), and I agree you'd never use that redirect value again, so modifying doesn't actually affect anything.

[alisd23]

@pshrmn Yeah actually I agree a Match inside a Miss can be done in other ways like using parent components. That makes the <LocationSubscriber> fix pointless, so forget about that 👍

[alisd23]

I'm happy with this! Best have someone else review this properly as well though, as it's quite a big change.

[alisd23]

@timdorr want to take a look at this?

[timdorr]

I will, but after this weekend. Crazy busy and I want to give your 3.0 PR some attention so we can ship that version.

[alisd23]

Sure, no pressure 👍

[ryanflorence]

Please see 227a2ed before doing anymore work in here.

[ryanflorence]

Basically, we can't use componentWillMount to "register" anything. We have to wait until the rendering is complete.

Yes, we can do it today and it "works" with componentWillMount, but I've talked to @sebmarkbage about this at length and writing code that is order dependent on componentWillMount is relying on implementation details of React, not specific contracts. And in "Fibers" these implementation details won't be there anymore and the 4.0 API would break.

In other words, we have to do a two-pass server render for exceptional cases (like 404s), and we just have to deal with not having the same checksum for now. Also see #3877 (comment)

That said, I'm confident we'll eventually figure out how to create an API like Match/Miss without the caveat.

[pshrmn]

@ryanflorence which componentWillMount are you referring to? The ones in this code are only used for server rendering (the same as the current code). Technically, I do use a cWM for setting the initial match state value for <Match> components, but that could also be done in the constructor and isn't involved in registering the component so it shouldn't matter.

[pshrmn]

The only thing that is order dependent is when serverRouterIndex is assigned (see code at [0] and [1]). Is there some situation where two renderToString calls of the same component will result in a different rendering order?

If there is, then matchContexts (in the server context object) would need to be an object and <MatchProvider>s would need to generate some sort of key to identify themselves.

The only reason that I can think of for non-deterministic render order would be if someone is randomly shuffling their components.

[0] https://github.com/pshrmn/react-router/blob/matchprovider-location/modules/MatchProvider.js#L60-L61
[1] https://github.com/pshrmn/react-router/blob/matchprovider-location/modules/createServerRenderContext.js#L16

[alisd23]

Thankfully I think randomly ordering components would be a very rare case, and it can be solved by doing the randomization on the actual data, before both renders.

The componentWillMount logic that you couldn't directly move to a constructor would be:

• In <Match>, location is grabbed off props or context then, setState is called with the match information. You can't get access to props or context in the constructor however.

The logic in <StaticRouter> (which was there already) is probably OK for now as it is not depended on by other components. It only needs to run before that component renders.

[pshrmn]

The arguments to the constructor are props and context, so the <Match> code be re-written as:

class Match extends React.Component {
  constructor(props, context) {
    super(props, context)
    this.state = {
      match: this.matchCurrent(props.location || context.location)
    }
  }

  componentWillMount() {
    const { match } = this.state
    const { serverRouter, provider } = this.context
    if (serverRouter && provider) {
      // ...
    }
  }
}

super just needs to be called before this.matchCurrent so that the this variables in matchCurrent are accessible.

That setState in componentWillMount running without the component actually mounting won't have any side effects, so I don't think that the state needs to be set in the constructor, but it is an option.

The randomization was the only instance I could think of which could break knowing the render order, but I'd be hard pressed to come up with a legitimate reason to do that. Since the reconciliation between server and client rendering fails if the server returns second pass code, I'm not sure that its necessary to address anyways.

[alisd23]

@pshrmn Oh yeah - forgot about that, my brain failed me there...

[pshrmn]

This could be optimized to check if the new match matches the current this.state.match and not calling this.setState if it does.

[ryanflorence]

what's the motivation here?

This is a way to manage location changes that is resistant to shouldComponentUpdate.

Now that we're using LocationSubscriber in Miss it survives sCU, is there other motivations here? I'm hesitant for a refactor like this before release, but also not against it.

Might be able to go the other way and use react broadcast for match on context instead of setting up our own event emitter in MatchProvider

[pshrmn]

(Sorry, this is a bit long)

The original intent was that the hierarchy of RR boils down to:

StaticRouter -> MatchProvider -> Match/Miss

Current Model

A <MatchProvider> is in charge of determining if its <Miss> components should render based on the state of its <Match> components. However, when the location changes, the subscription model bypasses the <MatchProvider>.

The only way that a <MatchProvider> knows that the location has changed is if a <Match> calls its addMatch or removeMatch methods. Because of this, it cannot control when it notifies its <Miss>es whether or not they should render.

Normally, this is not an issue because the <MatchProvider>'s cDU will be called after its children have updated (and called addMatch/removeMatch to set the correct match state). cDU then calls notifySubscribers, and any <Miss> components will be updated.

StaticRouter -> MatchProvider -> Match/Miss -> Match reports if it matches
  -> MatchProvider did update -> notify subscribers -> update Misses

When a component that calls sCU is placed between a <MatchProvider> and a <Match>, the <MatchProvider>'s cDU is triggered before the <Match> is updated by the <LocationSubscriber>. Then, when the <Match> calls addMatch/removeMatch, the <MatchProvider> will internally contain the correct state, but it won't notify its <Miss> components of this new state.

// initial update
StaticRouter -> MatchProvider -> should component update? false
  -> MatchProvider did update -> notify subscribers with incorrect match state

// then, location subscriber's queued setState update executes
LocationSubscriber -> Match -> addMatch/removeMatch

Now, a duct tape fix would be to modify addMatch and removeMatch so that they call notifySubscribers every time that they are called. This works, but I believe that the abstraction of bypassing the <MatchProvider> on location updates is still wrong.

MatchProvider is location aware

Note: while writing this I realized that the <Match> doesn't actually need the <LocationSubscriber> with this model.

When the <MatchProvider> is location aware, all location changes pass through the <MatchProvider>. When there is no sCU, this means that it will function similarly to the current model.

StaticRouter -> MatchProvider -> Match/Miss -> MatchProvider did update
  -> notify subscribers -> update Matches (this is new) -> update Misses

The "magic" behind this is that <MatchProvider> always updates its <Match>es and <Miss>es when it updates. The root <MatchProvider> (child of <StaticRouter>) will never be stuck behind a sCU, so all of its <Match>es and <Miss>es will always update. Then, because each of those <Match>es updates, its <MatchProvider> child will also update. So on and so forth, each <MatchProvider> will be updated and hence so will its children.

StaticRouter -> MatchProvider -> should component update? false
  -> MatchProvider did update -> notify subscribers -> update Matches
  -> update Misses

For a normal update cycle (no sCU) this causes us to double render our <Match>es instead of just our <Miss>es. However, each <Match> keeps its match in state, so we can optimize this by only calling setState if this value is different, preventing the re-render.

In summary

By keeping the location in <MatchProvider> and updating our <Match> and <Miss> components whenever the <MatchProvider> updates, we can ensure that they are always rendering based on the current location.

This should simplify the logic of RR because location changes flow in the same direction as the hierarchy.

@ryanflorence If you decide to go this route, the createServerRenderContext changes need a code review. I added some tests, but essentially I believe that while the existing code passed the tests, it didn't really work.

[alisd23]

@pshrmn Just to remind me (completely forgotten how this all works now), this will still require 2 render passes to render a miss?

Seeing is this is quite a major and general change to the MatchProvider logic, maybe worth looking at the 2 pass <Miss> render issue as well. Is there any reason the addMatch and addMiss logic (in Match and Miss) can't be in the constructor, instead of in cDM?
Surely even in React Fiber the constructor will only called exactly once at creation, and componentWillUnmount will only be called exactly once before unmounting

Also conflicts 😄

[pshrmn]

@alisd23 From sebmarkbage's comment facebook/react#7671 (comment)

1. It is confusing when used with error boundaries because currently componentWillUnmount can be called without componentDidMount ever being called. componentWill* is a false promise until all the children have successfully completed. Currently, this only applies when error boundaries are used but we'll probably want to revert this decision and simply not call componentWillUnmount here.
2. The Fiber experiment doesn't really have a good way to call componentWillUnmount when a new render gets aborted because a higher priority update interrupted it. Similarly, our sister project ComponentKit does reconciliation in threads where it is not safe to perform side-effects yet.

With Fibers we cannot rely on cWU being called, so subscribing to a <MatchProvider> before we know that a <Match> or <Miss> has mounted means memory leaks if the component never mounts.

There really is no way to determine if a <Miss> should render in the middle of the first pass using only React. Its an order of operations thing, where <Miss> requires information about all of its <Match> siblings (per <MatchProvider>, so these can also be nested inside of other components) to determine if it should render.

A non-React example would be if we want to walk through an array and call a largestNumber(n) function when we find the largest number. The largest number is 9, but on the first pass when we encounter 9, we have no guarantee that no number after it is larger, so we keep track of the current largest number and call largestNumber(9) after we fully iterate the array.

[1, 7, [9], [5, 6], 3, 8]

Now, it is possible to have a <Miss> render on the first pass:

1. Create a route config with the same structure as your <Match> and <Miss> components.
2. Create a function that takes a location and walks through the route config. It will mark each component as true/false depending on whether they match the current location. The miss routes cannot be set on the first pass because we don't know whether all of its siblings, so we register callbacks to set them once we have finished checking all of its siblings.
3. The miss callbacks are called and we now have a route config that has the correct state for whether or not each route matches.
4. Now we can render our React app. For each <Match> and <Miss> component, we need to find the associated route in our route config object. Once we do that, we just have to check if its true/false to determine if it should be rendered.

It's a PITA because you're basically duplicating your app and React just so that the first pass on the client matches the second pass output of React on the server.

<MatchGroup> somewhat solves this, but it requires you to structure your app in a specific pattern.

[alisd23]

Oh so it's componentWillUnmount which is the problem as well. Damn.

With the MatchGroup, thankfully I think being forced to order Miss components after Match components is fine - I can't think of any reason why you need to do something other than that. But as soon as you wrap a Match in another component I feel like there'd be issues. You'd have to enforce that top level components should take a pattern parameter.

[pshrmn]

While I think that this approach could work, it gets a bit complicated when dealing with managing async components and it might be easier to handle that with calling notifySubscribers every time a <Match> adds/removes itself. Personally I am a fan of there not being a <Miss> component, which would make these changes moot.

Either way, I'm going to close this.

[jedwards1211]

@pshrmn @ryanflorence do you guys think using some kind of <LocationSubscriber>s is worth looking into again now that the <Miss> component has been removed? I would really like to avoid having to worry about shouldComponentUpdate defeating location changes.

[timdorr]

That's what withRouter will do for you.

[jedwards1211]

@timdorr I get that but it's still and extra thing we have to do manually. if Route, Switch, Link etc would subscribe to the location via context, manual intervention would be necessary in any case. What are the downsides to subscribing?

[jedwards1211]

*would not be necessary

[pshrmn]

Subscriptions do not work well in a decentralized system because of React's context model. Redux is centralized in the form of a single store, but with React Router, you can have multiple route matches (so components have to know which route to get context data from).

https://medium.com/@pshrmn/ditching-subscriptions-in-react-router-6496c71ce4ec

I wrote this article a while ago, and while it is probably a bit longer than it needs to be, I think that it pretty thoroughly covers why React Router no longer does subscriptions (and why that isn't a bad thing!)

[jedwards1211]

I understand it's decentralized, but wouldn't it be pretty easy to make every component that puts a location on its child context put an observable location on its child context as well or instead, triggering changes on the observable whenever it puts a different location on its child context?

[jedwards1211]

On second thought...I guess what you're saying is the problem is that even an observable location could get replaced with another instance of an observable location, but child components beneath a shouldComponentUpdate blocker wouldn't know to unsubscribe from the old one?

Wait no, I can't imagine a case in which that would happen, because a new instance of a parent component and its observable location instance wouldn't get created without the descendant's componentWillUnmount getting called first.

So far I've never encountered a case where I wanted to pass down dynamic values via context that I couldn't solve by putting some observable-type thing on context.

[jedwards1211]

Now I realize, even if an observable location is on each Route's child context, handling location changes in user-written components (the kind that currently take the current location as a prop) wouldn't be so simple.