Don't create a new history entry when transitioning to the current path

[agundermann]

This PR normalizes the behavior of all histories to not create a new history entry when pushing the path that's already active. Like browsers do with static content.

Currently, all histories create new entries, except HashHistory without queryKey since it's unable to do so. Instead, it'll print a warning which doesn't seem right considering it's a common scenario.

I implemented it in the history's finishTransition methods to treat these cases like a REPLACE. location.action will still be PUSH though. Alternatively, I think this could be implemented in the base history to change location.action to REPLACE. That's what I was doing in an older PR.

Also related: remix-run/react-router#1031

[mjackson]

Alternatively, I think this could be implemented in the base history to change location.action to REPLACE

This feels more semantically correct to me. Just because pushState was called doesn't mean we necessarily need to emit a push location. If we're really just doing a replace behind the scenes, we should probably let the user know.

[agundermann]

You're probably right. I was thinking with PUSH one might still be able to figure out what happened if we add location.current or location.delta.

I changed the implementation. Let me know if/how you want this tested.

[ricardosoeiro]

Is this PR going to be included? I think it makes more sense than the current default...

[mjackson]

Yes, I'd definitely like to move forward here. Just haven't had time to take a look!

@taurose I'm ok with whatever tests you'd like to write here. Maybe we could write a test that only tests hash history and does a pushState and then checks to make sure location.action is actually a REPLACE?

Sorry for taking so long to come back around here.

[th0r]

Guys, maybe add a REFRESH action?

In my project I've made a useRefresh middleware that adds history.refresh() method that calls history.listen handler with the current location, but sets it's action to REFRESH.

I need it to softly "reload" the page (refetch all the routes data) and it would be great to be the part of the core.

Here is this middleware if somebody needs it:

/**
 * Adds `history.refresh()` method that will call `history.listen` handler again with the current location and `REFRESH` action
 *
 * @param {function} createHistory
 * @returns {object} - `history` object
 */
function useRefresh(createHistory) {
    return opts => {
        const history = createHistory(opts);

        if (!history.listen) {
            throw new Error('History should provide "listen" method');
        }

        let currentLocation;
        let originalListener;

        function listen(listener) {
            originalListener = listener;
            history.listen(location => {
                currentLocation = location;
                listener(location);
            });
        }

        function refresh() {
            if (!originalListener || !currentLocation) {
                return;
            }

            currentLocation.action = 'REFRESH';
            originalListener(currentLocation);
        }

        return {
            ...history,
            listen,
            refresh
        };
    };
}

module.exports = useRefresh;

[agundermann]

Just rebased and added a test that checks if pushState to the same path will result in a REPLACE action.

@mjackson: Why only test hash history? That behavior affects all histories. I had to do some changes to hash history to not print a warning, but I don't think it makes sense to test this.

@th0r: This wouldn't re-execute react-router transition hooks, would it? Either way, I think this should be a separate feature request.

[ricardosoeiro]

Looking good :)

[taion]

@taurose @mjackson Ping! Should we rebase and merge this?

[ricardosoeiro]

Still waiting for this PR... how can I help, to move this forward?

[agundermann]

No objections here. I was hoping for @mjackson or someone else to take another look :)

[NeXTs]

+1 waiting for this PR

[mjackson]

I'm taking a look at this now, @taurose. Thanks for your patience :)

[mjackson]

I'm not 100% sure this is the right place to do this, but should be ok for now. My concern is for environments where we may not be able to synchronously call getCurrentLocation, like native, where we have to use AsyncStorage to store everything.

[agundermann]

I think in that case we'd want to hold on to the current location (or at least the path).

[taion]

Why is it correct to use getCurrentLocation here rather than just location?

[agundermann]

@mjackson I just realized that I haven't considered location.hash. Seems like the two lines here should use createPath(location) instead of concatenation, right?

[taion]

I think createPath is the correct check here. It doesn't account for basename (and that part is a bit messy honestly in createBrowserHistory), but basename isn't going to change on us anyway, so it doesn't matter. That would also fix #43 (comment).

[joshacheson]

Any chance on a timeline for this getting published to npm? Using history in production, and this fix will be a huge help in solving some problems i've been having caused by redux-router / react-router and hash histories

[just-boris]

@taion well, when these changes will be published on npm? It is so hacky to download package directly from github

[just-boris]

@taurose now I am about the history package. Or this question is not for you?

[taion]

Oops, disregard what I said earlier - I got confused about which issue I was on.

[taion]

Yeah it would be pretty nice to get this released on npm when possible.

[mjackson]

Just released in version 1.13.1. Enjoy.

On Fri, Nov 13, 2015 at 2:59 AM Jimmy Jia [email protected] wrote:

Yeah it would be pretty nice to get this released on npm when possible.

—
Reply to this email directly or view it on GitHub
#43 (comment).

[NeXTs]

@taurose Thank you so much! Finally no more annoying messages at console with redux-router and hash history

[just-boris]

Tested on my app too. Working fine, thanks!

[phuson]

I have a case where I have a bunch of anchors on a page, and want to record when a user navigates between those anchors. With the change in this PR, it removes that functionality, because now it treats all hash changes on the same page as if they're one single item in the browser history. Is there a way to optionally allow the old functionality for those of us who need it? Alternately, is there a way to force a pushState for items we want to maintain in the history, instead of automatically replacing the states if the URLs are considered to be the same?

[taion]

@phuson

That doesn't sound good. Seems like we should be checking the full computed path instead.

Would you mind opening this as a new issue? It looks like a regression introduced here, and it'd be better to track it separately.

[taion]

@taurose Do you think you'll have a chance to address the issues? If not, I can amend the code in question.

[phuson]

@taion Thanks for looking into this! Here's the new issue for tracking purpose: #166

[taion]

@taurose Never mind, I put together a PR with the fix (: