Handle routing state and replace

[kimjoar]

A couple of different changes here. Could split into multiple PRs, but wanted to see if you're interested first (I've also of course seen the PRs that already exist, but just copied this from the changes I needed for my app)

The changes:

• No longer use window.location, always rely on the location we receive from history
• Because of the above, let the initial state not be the current url, but set is with noRouterUpdate=true
• Add handling of location.state (and therefore a deep equal check when comparing locations)
• Use history.createPath instead of locationToString
• Make it possible to replace in addition to push

Also thought about renaming, but didn't for this PR:

• updatePath to pushPath (closer to history.push and history.replace)
• noRouterUpdate to skipUpdate or noUpdate
• syncReduxAndRouter to syncReduxAndHistory

[jlongster]

This is looking good. I really don't want to do a deep comparison on state though; luckily, I figured out they comparing locations is the wrong way to detect changes anyway. See the new sync logic at #40. I'm going to merge it after I write some basic tests.

I need this to be rebased on top of that once I land that. Thanks a lot for this PR! Either go ahead and rebase or I'll ping you when it's merged.

[jlongster]

Also see #21 (comment). I'm not sure if we want to change actions names or not.

[MattKunze]

Since this is already changing the API for updatePath, any thoughts on losing the ordinal boolean parameters? updatePath( path, null, true ) doesn't read as well as updatePath( path, null, { noRouterUpdate: true } )

Or for that matter, since state will be optional most of the time the signature could be function updatePath( path, { state, noRouterUpdate } = {} )

[jlongster]

I'm down with that. Let's go ahead and change the API to pushPath and replacePath. Does that sound good? Or "route" instead of "path".

[MattKunze]

Matching the naming scheme used by history makes the most sense to me, but it sounds like they might be renaming things as well (to just push and replace) so who knows.

[jlongster]

Yeah, that's what Ryan said. Also I don't think pushState really says anything about routing outside of react-router, so seeing that action in the wild isn't the best name IMHO, but push and replace are even more vague. I don't think there's anything wrong adding more semantics into the name, and they obviously map to push/replace.

[jlongster]

I just landed the new sync logic. Can you rebase?

[jlongster]

And also add tests!

[kimjoar]

@jlongster Great — I'll rebase and fix the tests asap

[ellbee]

I think this PR will reintroduce the problem with devtools where a 'RESET' action will change the path to '/' regardless of the initial URL. In my opinion the benefits of landing this outweigh this devtools regression, but it would be nice if we could figure out a way round it.

[ellbee]

This is the PR where it was originally "fixed" (modulo my typo of not quoting undefined) #6

[jlongster]

@ellbee that's true, I didn't notice we lost that check. Should be easy to add back in the check to see if we have a window. However, I've been thinking that getting the URL from window.location isn't the best either. Because if you are using hash history you just want to get /foo, not /#/foo?_key=blah.

Maybe what we should do is set initialState to null/false/etc, but in the initial history.listen call (the listener is always called immediately when you add a listener) we do initialState = { path: locationToString(location), ... } and change the initialState. We wouldn't be mutating the object, we would just define it with let so we could override it later.

[jlongster]

Although I guess we'd have to keep track if we've set the initialState or not and not do it multiple times... I don't love it, but history does not have a way to get the "current location".

[kimjoar]

@jlongster Rebased and added tests. I also fixed a bug with avoidRouterUpdate. Let me know what you think.

(I thought about cleaning up a couple of things in the tests, but I'll do that in a follow-up PR — this is already a big change)

[jlongster]

Are you sure we need to deepEqual here? I'm having a hard time wrapping my head around this, but probably because I haven't use that state much.

Hm, I have a thought... read my 2nd comment below.

[kimjoar]

It's basically a copy of https://github.com/rackt/history/blob/master/modules/createHistory.js#L12-L18

[jlongster]

Thanks a bunch for this, it's awesome! I'll try to merge it soon, just wanted to look through it once more. It is thanksgiving also :)

[kimjoar]

@jlongster Thanks for the comments — I'll take a look and try to get it fixed without the deep-equal :)

[jlongster]

What was the bug with avoidRouterUpdate btw?

[kimjoar]

@jlongster The bug: kimjoar@a0d6945

It's specified as updatePath(path, avoidRouterUpdate), but used as an object updatePath(routePath, { avoidRouterUpdate: true }).

Btw, pushed a test that would fail if we remove the locationsAreEqual check without other changes (too many store updates plus it changes replace -> push in the store). I'll look at ways of removing locationsAreEqual

[ellbee]

Although I guess we'd have to keep track if we've set the initialState or not and not do it multiple times... I don't love it, but history does not have a way to get the "current location".

Ok, thanks. That sounds like a good plan.

[jlongster]

Thanks all. I have to leave and it's Thanksgiving so I'm going to be slow to reply over the next day or two. I'm happy to merge this in with deep-equal for now and then we can look at it later as well. @kjbekkelund is this ready to merge if we just go with deep-equal for now?

[kimjoar]

@jlongster Everything looks ok when testing in my app, so it should be ready to merge. Have a great Thanksgiving!

[kimjoar]

I found a way to remove deep-equal. I think we need to be sure it's the right way to go, so it should be its own PR (e.g. to discuss additional tests etc). I'll create a PR against master when this is merged.

[jlongster]

Cool! I think I'm going to wait until tomorrow night to merge. We got a lot of attention today and I just don't want to merge something big right before a day when I'm not going to be around, even though it's just the master branch.

[kimjoar]

👍 Relax and enjoy Thanksgiving!

Got some ideas to solve deep-equal, but I think we need tests running in browsers to ensure everything still works as expected.

[jlongster]

Alright let's just merge this now and clean it up over the next few days for a release.

[jlongster]

Thanks a lot for all the work @kjbekkelund and others!