Make `params` available to getChildRoutes providers #3556

avanderhoorn · 2016-06-20T18:23:11Z

As it says on the label, this is an initial POC of making params and remainingPathname available to getChildRoutes providers. This was instigated primarily via a discussion with @taion and a another minor discussion with @timdorr.

Use Case
The use case this is intended to help with is in cases where the child route to be returned by getChildRoutes is dependent on the params that have been evaluated so far. For instance, if I have :country as the path for the current route being matched, in the getChildRoutes function for that route, I want to be able to access the country param so that I can decide which child route should be returned for that country (it can be different in different cases). Additionally, I have other cases where I want to "read forward" into remainingPathname of the path to decide what child route should be returned. The later being a more minor use case.

Currently, without this patch, the only context I have available is the location arg, which is not that useful and which I end up having to parse manually. This is doable, but is tedious and leads to very brittle code (i.e. if you add/remove "upstream" path segments).

Approach
As discussed with @taion, this PR has been done in a "non breaking" fashion - it follows the same pattern as a similar commit which was done to getComponent - here. This is done by making all the location properties available at the root level for non dev builds but adding warnings to usage of those properties in dev builds (again following the same logic as with getComponent).

At the time of calling getChildRoutes we have access to the current params that have been available (which is what I am primarily after) and the remainder of the path that needs to yet to be parsed. As such, a state object is created with these values, plus the additional location properties, and passed into the registered getChildRoutes.

Questions

progressState vs nextState - Elsewhere in the system, this object is called nextState, I wasn't sure you would consider if this exactly matches the semantics at play here since technically its not the "final" state and doesn't match the same structure as the current nextState. Hence I have called this progressState. Do you agree with this, is there a better name or should it just be nextState?

Note
I have abstracted out the deprecateLocationProperties function from getComponents as the logic is exactly the same. I'm assuming this is appropriate/desirable.

Work to be done
Assuming we are good with this PR and/or get to a point where we are good with it, I will finish off the, tests, samples update, docs, etc (inline with the work that was done with the getComponent commit mentioned earlier).

…ildRoutes providers

timdorr · 2016-06-20T18:40:02Z

I think this looks good. It just needs tests and docs updates, as you've pointed out.

Keep in mind a lot of this is obsoleted by 3.0, which we should have ready to go in a few weeks. But if the upgrade path turns out to be too difficult or burdensome (which is going to be inevitable, as it involves some major API changes), then it will be good to include this in 2.0 so you're not forced to upgrade for one minor feature.

taion · 2016-06-20T19:10:37Z

Let's try our best to minimize API changes for the less advanced functionality... the getComponents API ought to be replaced (and we have other issues discussing that), but we should e.g. try not to break people using synchronous route configs with basic routing and maybe one or two middlewares.

avanderhoorn · 2016-06-20T19:29:35Z

@timdorr Is there anything that talks about whats going to be in v3, etc? I can see some pr's against the v3 milestone, but it doesn't give much of a picture of whats happening or what the scope is.

@taion Was that comment directed at me and this pr? Are you saying that this is ok?

timdorr · 2016-06-20T19:32:50Z

Nope, Ryan and Michael are still finalizing things and don't want to go public until things are rock solid (which more important given the visibility of this project).

avanderhoorn · 2016-06-20T19:34:30Z

Ok. So given that you probably know more than me about whats coming, do you think its worth the effort to finish this off still?

taion · 2016-06-20T19:36:26Z

modules/matchRoutes.js

@@ -160,7 +162,19 @@ function matchRouteDeep(
      }
    }

-    const result = getChildRoutes(route, location, onChildRoutes)
+    const progressState = { 


In most cases the route will not use this object; no reason to do stuff like call createParams unless we know we need to call getChildRoutes.

Ok so you would like the call to createParams shifted to between lines 15-18?

timdorr · 2016-06-20T19:39:19Z

@avanderhoorn Yes, definitely. For those that don't want to or can't upgrade to 3.0, but would need something like this.

avanderhoorn · 2016-06-20T19:41:44Z

Ok great!

avanderhoorn · 2016-06-20T20:11:06Z

@taion I've updated given your feedback. Does this look like what you where thinking?

avanderhoorn · 2016-06-20T20:13:32Z

Also are we happy with the usage of progressState before I start going through and updating things elsewhere?

taion · 2016-06-20T20:40:40Z

modules/matchRoutes.js

  if (route.childRoutes) {
    return [ null, route.childRoutes ]
  }
  if (!route.getChildRoutes) {
    return []
  }

-  let sync = true, result
+  let sync = true, result, progressStateWithLocation
+  const progressState = { 


I would call this partialNextState – we use the term elsewhere for partial next state.

Can we omit remainingPathname here? I don't think it makes sense to pass down at this point. It feels odd for child routes to explicitly depend on the not-yet-matched portion of a route.

partialNextState is fine.

taion · 2016-06-20T21:46:11Z

This is great. LGTM. Thanks.

avanderhoorn · 2016-06-20T21:58:31Z

Added unit test here. I think thats everything. Any feedback?

taion · 2016-06-20T22:07:27Z

modules/__tests__/matchRoutes-test.js

@@ -332,6 +332,35 @@ describe('matchRoutes', function () {
    })
  })

+  describe('a nested route with a getChildRoutes callback', function () {


Can you combine this with the previous describe() block? They're really covering the same thing.

timdorr · 2016-06-20T22:07:30Z

👍

avanderhoorn · 2016-06-21T00:04:09Z

Ok I think this is the final update.

avanderhoorn · 2016-06-21T04:20:29Z

The value of state persists between test invocations, which could lead to potential problems. Instead use createSpy and assert on spy.calls[0].arguments[0].

Can you show me how you would like this done in this context?

taion · 2016-06-21T04:23:50Z

modules/__tests__/matchRoutes-test.js


    beforeEach(function () {
-      getChildRoutes = function (location, callback) {
+      getChildRoutes = function (partialNextState, callback) {


Use expect.createSpy here: https://github.com/mjackson/expect#spies

You can then assert on getChildRoutes.calls[0].arguments[0]

avanderhoorn · 2016-06-21T05:43:24Z

Ok I think thats it

taion · 2016-06-21T05:43:56Z

This is fine – I'll resolve the remaining test issues myself.

@reactjs/routing Are we okay with cutting a maintenance 2.5.0 with this change?

avanderhoorn · 2016-06-21T17:08:47Z

Thanks for your help @taion. Looking at that test, it now makes sense. Also rolling it out to the other APIs is a plus. Regarding the refactoring, I was going to do that, but thought it was a bigger change and was wanting to get the core concept in first.

Initial POC of making params and remainingPathname available to getCh…

a81788b

…ildRoutes providers

taion reviewed Jun 20, 2016
View reviewed changes

Update given feedback on when to call createParams

2c90d04

avanderhoorn added 2 commits June 20, 2016 13:32

Update documentation

8e2576f

Update samples and tests usage of location to progressState

61c64f7

taion reviewed Jun 20, 2016
View reviewed changes

avanderhoorn added 2 commits June 20, 2016 14:02

Rename from partialState to partialNextState

d3b7ce5

Remove usage of remainingPathname for the moment

bfb18e6

Add unit tests

400734e

taion reviewed Jun 20, 2016
View reviewed changes

Update and consolidate tests

b8c1820

avanderhoorn changed the title ~~Make params & remainingPathname available to getChildRoutes providers~~ Make params available to getChildRoutes providers Jun 21, 2016

Fixup whitespace issues

3bf3fa9

taion reviewed Jun 21, 2016
View reviewed changes

avanderhoorn added 2 commits June 20, 2016 22:38

Setup spy to observe changes

414b612

Fix extra white space

f566bf4

avanderhoorn added 2 commits June 20, 2016 22:41

Hopefully last one

56353f4

Nope 2 more

2aa9a9e

taion merged commit c1167fb into remix-run:master Jun 21, 2016

taion added a commit that referenced this pull request Jun 21, 2016

Fix remaining issues with #3556

d62de8d

timdorr pushed a commit that referenced this pull request Jun 21, 2016

Fix remaining issues with #3556 (#3561)

f9f5a88

taion mentioned this pull request Jun 22, 2016

Put location on partialNextState #3569

Merged

renovate bot mentioned this pull request Oct 11, 2018

Update dependency react-router to v2.8.1 - autoclosed neolanders/min-starter-react-redux-axios#14

Closed

1 task

lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `params` available to getChildRoutes providers #3556

Make `params` available to getChildRoutes providers #3556

avanderhoorn commented Jun 20, 2016

timdorr commented Jun 20, 2016

taion commented Jun 20, 2016 •

edited

Loading

avanderhoorn commented Jun 20, 2016

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

avanderhoorn Jun 20, 2016

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

avanderhoorn Jun 20, 2016

taion commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 21, 2016

avanderhoorn commented Jun 21, 2016

taion Jun 21, 2016

avanderhoorn commented Jun 21, 2016

taion commented Jun 21, 2016

avanderhoorn commented Jun 21, 2016

Make params available to getChildRoutes providers #3556

Make params available to getChildRoutes providers #3556

Conversation

avanderhoorn commented Jun 20, 2016

timdorr commented Jun 20, 2016

taion commented Jun 20, 2016 • edited Loading

avanderhoorn commented Jun 20, 2016

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

Choose a reason for hiding this comment

avanderhoorn Jun 20, 2016

Choose a reason for hiding this comment

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

Choose a reason for hiding this comment

avanderhoorn Jun 20, 2016

Choose a reason for hiding this comment

taion commented Jun 20, 2016

avanderhoorn commented Jun 20, 2016

taion Jun 20, 2016

Choose a reason for hiding this comment

timdorr commented Jun 20, 2016

avanderhoorn commented Jun 21, 2016

avanderhoorn commented Jun 21, 2016

taion Jun 21, 2016

Choose a reason for hiding this comment

avanderhoorn commented Jun 21, 2016

taion commented Jun 21, 2016

avanderhoorn commented Jun 21, 2016

Make `params` available to getChildRoutes providers #3556

Make `params` available to getChildRoutes providers #3556

taion commented Jun 20, 2016 •

edited

Loading