search

mozilla / addons

☂ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

Update to react-router-4 #10295

Closed tofumatt closed 6 years ago

tofumatt commented 7 years ago

This will be a fair bit of work but filing here as an issue on our radar, so we can close the Greenkeeper PRs and track it here.

kumar303 commented 7 years ago

It took me a while to wrap my head around this one but now I've hit a road block. redux-async-connect relies on the old router API and since it declares react-router as a peer dep, it doesn't have its own isolated copy.

We could fork it and hack package.json to declare it as a dep but eyeballing what it does with ReactRouter tells me that it still may not work. It looks like we need to replace redux-async-connect with redux-saga so let's wait on the router upgrade until then.

kumar303 commented 7 years ago

Here's my patch that was very broken. I doubt we'll need to reference it but maybe. react-router-4-upgrade.patch.txt

kumar303 commented 7 years ago

Here is the migration guide: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/migrating.md

kumar303 commented 6 years ago

I've been trying to connect components to react-redux-router's state but this is apparently unsupported because the state is not always in sync with react-router (wtf). After reading some issues, react-router seems like a big mess.

I've been looking at http://router5.github.io/ which may be a good alternative. The redux support looks more solid.

kumar303 commented 6 years ago

http://router5.github.io/docs/understanding-router5.html

[router5] also works very well with state containers like Redux: your state container is placed between your view and your router, and your view subscribes to state updates.

This is exactly what I want in a router ^

As far as I can tell, you can't do this with react-router-redux because react-router doesn't offer enough hooks. E.G. You still have to use route handlers for initial component rendering.

muffinresearch commented 6 years ago

@kumar303 if that provides all the nice things like regexes etc and is a better fit for our setup then I'm +1 on us moving in that direction.

willdurand commented 6 years ago

There is a new router library that looks interesting: https://reach.tech/router.

willdurand commented 6 years ago

For QA: the website should continue to work as expected, there should be no issue with links or page reload.

AlexandraMoga commented 6 years ago

I have made a general verification of the frontend on AMO -dev with focus on links and page reloads and I haven't come across any regressions, except for the issue mentioned in https://github.com/mozilla/addons/issues/12136

I'm marking the issue as verified fixed