Upgrade React Router to v4

openfisca / legislation-explorer

Explore legislation formulas and parameters.

https://legislation.demo.openfisca.org

GNU Affero General Public License v3.0

26 stars 12 forks source link

Upgrade React Router to v4 #203

Open fpagnoux opened 6 years ago

fpagnoux commented 6 years ago

There was several bugs recently (e.g. #202 #199 ) that we suspect were related to React Router.

Upgrading it needs to be done at some point anyway, and could save us trouble.

Morendil commented 6 years ago

I advise against for the reasons in #202 - it's entirely possible we'd only be changing who gets a head start in a race condition, leading to the dreaded "it works on my machine" effect. At the moment it repro's reliably on one machine (mine, at least this morning), I'll investigate on my spare time as this is a production bug.

Morendil commented 6 years ago

Also worth noting: "React Router v4 is a complete rewrite, so there is not a simple migration path."

It might save us trouble, but it will also definitely cause some.

bonjourmauko commented 6 years ago

Hi ! I think this issue is more a theme, a regroupment of several issues. It needs:

investigation
eventually feasibility testing
and slicing

There are several things we know we have to change :

changes to the history API
introduction of specialised history constructs
deprecation of nested routes
migration from monolith routes to component-based ones

And many other we do not know yet.

It is highly advisable to investigate and prototype up-front before investing a lot of time on this.

sandcha commented 5 years ago

A good react-router migration guide is available here.

After some feasibility testing on to-react-router-v4 branch, imho, the main issue is the lack of appropriate tests (on history for example) to understand how far we break the app at a migration step.

Morendil commented 5 years ago

The above guide is missing some details, for instance locationShape and routerShape are no longer available.

fpagnoux commented 5 years ago

Making this issue a "theme" feels a little much to me. If upgrading the router is such a big task that it's worth becoming a theme, then maybe it's just not worth doing it.

IMHO, a reasonable way to proceed if we are unsure about the complexity would be to time-box a fixed amount of time and see how it looks (what @sandcha seems to be doing 👍).

I'll also point out that the legislation explorer is a relatively simple app, with 4 types of pages, 3 of them just displaying static content. So I'd not be scared beyond measure about breaking something so subtle that we would not be able to identify it with some relatively simple tests.

fpagnoux commented 5 years ago

At the moment it repro's reliably on one machine (mine, at least this morning), I'll investigate on my spare time as this is a production bug.

If we can reliably reproduce the current bugs, then yay let's go for it and fix these before thinking about the upgrade.

Morendil commented 5 years ago

Let's deal with more urgent updates first: https://github.com/openfisca/legislation-explorer/network/alerts