Export getCurrentLocation

[jlongster]

There is a getCurrentLocation that the history object uses internally. Any chance we could export that so we can call history.getCurrentLocation()?

I was relying on window.location (I love the idea that you are creating compatible location objects) in redux-simple-router, but this breaks when using hash history. The URL that history sees is /foo, but window.location will report /#/foo?_k=blargh. See reactjs/react-router-redux#12 for the fun that ensues. (I ping Ryan and Michael in there but we can talk about it here)

I need to get the current location to compare it with the URL in the redux store to see if it has changed and we need to update the store. This allows the user to change the URL in the store and then we'll automatically trigger a history update. I suppose we could hack around this and somehow keep a "dirty" flag but this seems more elegant.

[taion]

Why not just save down the location object you get at https://github.com/jlongster/redux-simple-router/blob/master/src/index.js#L49?

One more note on that code, incidentally - due to remix-run/react-router#2502, your history listener actually doesn't necessarily fire in sync with the router update. When using async routes/components, you could potentially update your location significantly before the router update. I've seen this on other similar React Router + Flux integrations, and I'm not sure whether it's correct.

[jlongster]

I don't like for code to depend on external variables, that could potentially get out of sync. I guess saving the location object could work, but I like the beauty of each listener being completely isolated. And this seems like a pretty natural API. Why not make the state observable?

Thanks for the note about async routes. I'll have to look into those more. I probably will when I start adding tests and more examples. I have no idea when listen fires when using async routes, not even taking into account the useRoutes mess. (Major +1 from me to clearing that up and having something like listenRoutes)

[taion]

I don't think that's the right thing to do. For example, look at the impl here: https://github.com/rackt/history/blob/v1.13.1/modules/createBrowserHistory.js#L27-L45.

Especially if you want to key off router state rather than history state, and to avoid issues when we're using transition hooks or whatever, you're going to end up with something much more robust if you use the location you're given in the listener, rather than trying to figure it out from window or whatever.

This is exactly the approach <Router> itself takes, for one.

[jlongster]

Trying to figure it out from window is exactly what I'd like to avoid. :) Maybe that internal function is not the right one. Just something that returns this? https://github.com/rackt/history/blob/v1.13.1/modules/createHistory.js#L40 I don't see what's so wrong about getting the current location. The current location is whatever was last passed to the history.listen function.

I'm fine doing it that way I guess. I just thought the code before had some beauty in it.

[taion]

getCurrentLocation really is for setting up an initial location object - you can see it's not really used for anything else.

One reason not to expose history.location is that it's not guaranteed to exist - we don't initialize it until the first listener is attached, and it's undefined before then. If all the listeners get detached, then it also again becomes invalid.

I'm using just this pattern in scroll-behavior, and so far it's not really bothering me: https://github.com/rackt/scroll-behavior/blob/9479062458e56f7152126ea8ba6e06a788155bd7/modules/utils/createUseScroll.js

[jlongster]

Alright, sounds good. Doesn't bother me too much. It seemed like that location object is essentially meant to replace window.location, so making it readable seemed natural. But if it's more complicated than that it's fine. Thanks.

[taion]

It largely is, but I think the base history API is intended to be largely stateless (though the routing history API isn't so much).