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

Upgrade to React Router v4 #667

Merged
merged 3 commits into from Oct 13, 2017
Merged

Upgrade to React Router v4 #667

merged 3 commits into from Oct 13, 2017

Conversation

tech4him1
Copy link
Contributor

- Summary

Close #413.

Upgrade React Router (and react-router-redux) to version 4.

- Test plan

Manually test routing (unless someone else has a better testing method).

- Description for the changelog

Upgrade to React Router v4.

- A picture of a cute animal (not mandatory but encouraged)
ryan-grewell-95002

@tech4him1
Copy link
Contributor Author

The current problem I am having is that the location is passed as a context prop, not a true prop. So, for example, if you go from one collection to another, the URL will update, but the list of entries will not, as it still gets the original :name param.

Here is a document on the fixes: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md. @erquhart @Benaiah do either of you have any suggestions? I'm thinking the more performant approach would be better, just wrapping each component in a simple arrow function instead of trying to use withRouter everywhere.

@tech4him1 tech4him1 force-pushed the react-router-4 branch 3 times, most recently from 727be70 to 987147a Compare October 9, 2017 21:53
@tech4him1 tech4him1 changed the title WIP: Upgrade to React Router v4 Upgrade to React Router v4 Oct 9, 2017
This issue is due to the Redux `connect` wrapper around `<App/>`.
`connect` diffs changes in regular props to know when to update the
component, but doesn't check context props like `location`.
See
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md.
@erquhart
Copy link
Contributor

@tech4him1 is your last comment still an issue? That would make this WIP, just checking status.

@tech4him1
Copy link
Contributor Author

tech4him1 commented Oct 11, 2017

@erquhart For status, it seems to be working for me, so it is ready for review (not WIP). I would appreciate you reading that article, though, to make sure you agree with how I implemented the fix (d3366ab) and make sure no more fixes need to be done.

<Switch>
<Route exact path='/' component={DashboardPage} />
<Route exact path="/collections/:name" component={CollectionPage} />
<Route path="/collections/:name/entries/new" render={(props) => (<EntryPage {...props} newRecord={true}/>)} />
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing I'm not quite sure about is whether this is the best way to pass newRecord through here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most "proper" way to do this would be to move the actual logic for newRecord to EntryPage itself. @erquhart @Benaiah do you mind that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could clean it up a little: render={props => <EntryPage {...props} newRecord />}

I think the boolean prop will work that way, right?

@tech4him1
Copy link
Contributor Author

@erquhart @Benaiah I've just been reading through this: https://css-tricks.com/react-router-4/#article-header-id-4.
I'm wondering if this would be a better way to structure our routes -- it seems a lot cleaner.

@Benaiah
Copy link
Contributor

Benaiah commented Oct 12, 2017

@tech4him1 based on our conversation in Gitter (pasted below for future reference), I think that's fine in some situations, but only where there's a clear advantage to doing so. Either way, I'd like to review and merge this as is as we discussed in Gitter.

Benaiah Mischenko @Benaiah 12:17
from what I'm seeing that's more for pages that are really nested. It could clean up auth, but most of the other routes have totally separate pages for each different route match
you don't have the issue he has in the article of additions to URLs drilling further into a single page (like if CollectionPage and EntryPage were the same component)
maybe I'm missing something but that seems to be the primary advantage of what he's suggesting here

Caleb @tech4him1 12:18
I agree on that part of it.

Benaiah Mischenko @Benaiah 12:18
I'm kind of a fan of the flat routing too, it makes it easy to know that you haven't missed any routes somewhere when debugging that stuff
auth might be a good spot for that technique. I don't see anything that indicates you can't do both depending on what's more appropriate for the situation
but CollectionPage and EntryPage have no shared UI pieces that I'm aware of

Caleb @tech4him1 12:20
what about for newRecord with EntryPage

Benaiah Mischenko @Benaiah 12:21
ah, that might be a good one. though OTOH it means EntryPage is now concerned with routing where it wasn't before. I don't think that's too much of an issue with the top-level page components, but I don't want routing concerns to couple themselves more than one or two layers into the UI
so I think that may also be a good fit, but I wouldn't want to blindly use that approach for everything, since I'd like most of the UI to remain decoupled. does that make sense?
the rule of thumb I'm thinking of is that coupling the component to the router is probably ok if the name of the component ends with Page 😛

Copy link
Contributor

@Benaiah Benaiah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - code seems fine and manually testing the routing didn't bring up any regressions. 404s aren't working when they're a valid route for a nonexistent resource, but that's not a regression.

@erquhart erquhart merged commit dbe96d3 into master Oct 13, 2017
@erquhart erquhart deleted the react-router-4 branch October 13, 2017 01:10
@tech4him1 tech4him1 mentioned this pull request Oct 14, 2017
aquibm pushed a commit to aquibm/netlify-cms that referenced this pull request Oct 17, 2017
* Upgrade to React Router v4

* Fix pages not change when the URL was changed.

This issue is due to the Redux `connect` wrapper around `<App/>`.
`connect` diffs changes in regular props to know when to update the
component, but doesn't check context props like `location`.
See
https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md.

* Update to new `history` methods.
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

Successfully merging this pull request may close these issues.

None yet

3 participants