remix-run / react-router

Declarative routing for React
https://reactrouter.com
MIT License
53.11k stars 10.31k forks source link

Use replace instead of push on <Link> click when location is the same #5362

Closed jasonmacgowan closed 3 years ago

jasonmacgowan commented 7 years ago

Each time you click a <Link/>, it pushes to the browser history even if the location already matches. This differs from the default implementation of anchor tags as far as I can tell.

When a simple anchor tag is clicked 100 times, there is only one history entry <a href="/foo">Go to Foo</a>

If you did the same for <Link />, you have 100 history entries which makes back and forward button navigation difficult.

Proposing we don't push when the location already matches

mahesh-beehively commented 7 years ago

Same issue react-router-native

pnest commented 7 years ago

Is it an issue? You can check link yorself and set appropriate 'replace' attribiute value in

timdorr commented 7 years ago

Edit: I have seen the abundance of thumbs downs and acknowledge my stupidity here. Feel free to ignore this comment...

@pnest has the right idea. This is "fixable" with the replace prop.

The point of <Link> is to emulate <a>. A normal <a> that links to the same page you are already on will push new history entities to the stack. For instance, click the react-router link at the top of the page a bunch of times. You'll see that you have to click back multiple times to get back to this page. We want to maintain HTML semantics with our defaults, rather than reinvent them.

pshrmn commented 7 years ago

@timdorr I actually think that the behavior of that link is caused by GitHub's SPA working incorrectly. Quoting myself from here:

From the WHATWG browser spec

If the navigation was initiated with a URL that equals the browsing context's active document's URL

  1. Replace the current entry with a new entry representing the new resource and its Document object, related state, and the default scroll restoration mode of "auto".
  2. Traverse the history to the new entry.

Where "equals" is defined here (basically, (a.pathname + a.search + a.hash) === (b.pathname + b.search + b.hash))

Using the replace prop can work, but I think that it would be better if history identified when navigating to the same location and replaces instead of pushes. That was the behavior in v2/3, but it was lost in the history rewrite.

Since this is a history issue, this doesn't need to be reopened. If/when history is updated to add same location detection, the <Link> API can be updated but for the time being replace is good enough.

vladshcherbin commented 7 years ago

@timdorr actually, normal same page <a> links don't create duplicates, so current <Link /> behavior is wrong and differs from 99% of websites. Even RR3 was working correctly, only RR4 is not.

@pnest this is a problem because it differs from default behavior, you have to reinvent Link component in every project because of RR4 (and history v4) bug.

This is a really annoying bug in RR4.

mjackson commented 5 years ago

Re-opening this issue because this is still broken. Going to add this to the roadmap for our next minor release.

pshrmn commented 5 years ago

I think that there could be a temp fix in a minor release of React Router, but the best long term approach would be for history to handle this automatically (e.g. with a method that compares the new location with the current location to determine if it pushes/replaces).

guidobouman commented 5 years ago

You want to fix this in react-router, okay.

But is there maybe a path for other libraries that have have a concept of showing screens (like modals) to also integrate programatically with the same history stack that react-router uses?

Something along the lines of:

import { RouterAPI } from "react-router-dom";

// optional
const options = {
  replace: false,
  // ...
}

RouterAPI.go('...', options);
RouterAPI.link('...', options);
RouterAPI.navigate('...', options);
RouterAPI.emitLinkClick('...', options);
RouterAPI.immitateLinkClick('...', options);
pshrmn commented 5 years ago

@guidobouman React Router uses the history package for its navigation.

guidobouman commented 5 years ago

@pshrmn I know. I have built this functionality on top of history for previous projects. I have also been advocating the adoption of link in history for some years: https://github.com/ReactTraining/history/issues/470#issuecomment-359159797. But that's going against @mjackson's philosophies for the package. If we can't convince him in three years time to put it inside history, then I'm willing to adjust my approaches to something that might actually get merged and still fix the scenario I'm trying to solve.

stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

guidobouman commented 5 years ago

@mjackson If this is the tracking ticket for this feature I would like to prevent this from becoming stale. Any ETA on 5.2?

TrevorSayre commented 5 years ago

@guidobouman @mjackson maybe we could consider updating https://github.com/ReactTraining/react-router/blob/master/.github/stale.yml to include a label in the exemptLabels section such as roadmap. Issues that have been accepted as something to be worked on can be labeled as roadmap and will not be marked as stale (and possibly closed) unless the roadmap label is removed.

cloudever commented 5 years ago

I just use history.block API always following way:

function applyBrowserLocationBlocker(history: History) {
  let currentLocation = null

  history.block((location, action) => {
    const nextLocation = location.pathname + location.search

    if (action === 'PUSH') {
      if (currentLocation === nextLocation) {
        return false
      }
    }

    currentLocation = nextLocation
  })
}
ghost commented 5 years ago

I'm with @guidobouman I thought that was the intent of not completely owning the history responsibility, exposing historys state. I do agree that non-state changing reloads of the same page route should be consolidated if at all automatable, atleast with a flag to use replace for those that want their hands off the navigation completely. The logic could live safely in Link to verify before dispatching.

ghost commented 5 years ago

I want to be clear I am satisfied with this behavior and would prefer any changes to be behind a flag rather than having to lock at version, are there any others seeing the current behavior as a feature rather than a bug? This package was a life saver and it deserves appriciation. I love the conversations I see over here. Keep up the great work!

kiily commented 4 years ago

Is anyone still working on this feature ? or is the ticket currently stale ? Would be nice to have a way to force this behavior, should it be needed

Thank you

aleclarson commented 4 years ago

I made a hook that builds on @cloudever's workaround:

import { useHistory } from "react-router-dom";
import { useEffect } from "react";

export function useLocationBlocker() {
  const history = useHistory();
  useEffect(
    () =>
      history.block(
        (location, action) =>
          action !== "PUSH" ||
          getLocationId(location) !== getLocationId(history.location)
      ),
    [] // eslint-disable-line react-hooks/exhaustive-deps
  );
}

function getLocationId({ pathname, search, hash }) {
  return pathname + (search ? "?" + search : "") + (hash ? "#" + hash : "");
}

It would be great if a hook like this was baked into the library!

palashkaria commented 4 years ago

We've been using this patch from a previous issue in our app for a long time (~3 years, since we moved to v4), with this middleware history passed to :

currently on v5.1.2

https://github.com/ReactTraining/history/issues/470#issuecomment-363449663

rofrol commented 3 years ago

Repository demonstrating the use of LocationBlocker.js from https://github.com/ReactTraining/react-router/issues/5362#issuecomment-552174266:

import React from "react";
import { Switch, Route } from "react-router-dom";
import Contact from "./Contact/Contact";
import About from "./About/About";
import useLocationBlocker from "./LocationBlocker";

export default function Routes() {
  useLocationBlocker();
  return (
    <Switch>
      <Route path="/about">
        <About />
      </Route>
      <Route path="/contact">
        <Contact />
      </Route>
      <Route path="/">
        <h2>Home</h2>
      </Route>
    </Switch>
  );
}
akshay-nm commented 3 years ago

I implemented something in one of my projects to somehow get over this issue. I maintain the current and previous path in the app state somehow. Then every time a link is clicked on, I just:

Here's the component:

import React, { useCallback } from 'react'
import { useSelector } from 'react-redux'
import { useHistory } from 'react-router-dom'

const SafeLink = ({ to, children }) => {
  const previousPath = useSelector((state) => state.path.previous)
  const currentPath = useSelector((state) => state.path.current)
  const history = useHistory()

  const onSafeLinkClick = useCallback(() => {
    if (currentPath !== to)
      if (previousPath !== to) history.push(to)
      else history.goBack()
  }, [history, previousPath, currentPath, to])

  return <button onClick={onSafeLinkClick}>{children}</button>
}

export default SafeLink

I use redux for state hence the selectors. Here's what I use to keep track of route changes:

import { useEffect, useState } from 'react'
import { useDispatch } from 'react-redux'
import { useLocation } from 'react-router'
import { updateCurrentPath, updatePreviousPath } from '../app/pathSlice'

const PathTracker = () => {
  const location = useLocation()
  const [buffer, setBuffer] = useState([null, null])

  const dispatch = useDispatch()

  useEffect(() => {
    dispatch(updateCurrentPath(buffer[0]))
    dispatch(updatePreviousPath(buffer[1]))
  }, [buffer, dispatch])

  useEffect(() => {
    setBuffer((prev) => [location.pathname, prev[0]])
  }, [location, dispatch])
  return ''
}

export default PathTracker

and finally the redux slice (redux toolkit):

import { createSlice } from '@reduxjs/toolkit'

const initialState = {
  previous: null,
  current: null,
}
const slice = createSlice({
  name: 'path',
  initialState,
  reducers: {
    updatePreviousPath: (state, action) => {
      state.previous = action.payload
    },
    updateCurrentPath: (state, action) => {
      state.current = action.payload
    },
  },
})

export const { updatePreviousPath, updateCurrentPath } = slice.actions

export default slice.reducer

Edit: I have not thoroughly tested this, seems to be working well for me for now. Your milleage may vary.

jarodburchill commented 3 years ago

This is the work around I use... though as stated above, this really should be the default behavior for Link

import { Link, useLocation } from "react-router-dom";
...
const location = useLocation();
...
<Link to="/" replace={location.pathname === "/"}>
  Home
</Link>
Yokomon commented 3 years ago

This issue has also been extended to RR5... figured out the bug last night and thought it would have been fixed by now. @jarodburchill I'll try out your work around.

Yokomon commented 3 years ago

It seems replace fixed the issue on RR5, but i think this used to be a default behavior Link :thinking: . By default, when Link(RR5) is clicked on, history.push() is called. So if it is clicked on multiple times, it just keeps creating a new history on the browser which caused the annoying bug of pushing same location in the history object. So replacing .push() with .replace() could definitely resolve this issue. @mjackson should i make a PR to resolve this issue?

akshay-nm commented 3 years ago

It seems replace fixed the issue on RR5, but i think this used to be a default behavior Link 🤔 . By default, when Link(RR5) is clicked on, history.push() is called. So if it is clicked on multiple times, it just keeps creating a new history on the browser which caused the annoying bug of pushing same location in the history object. So replacing .push() with .replace() could definitely resolve this issue. @mjackson should i make a PR to resolve this issue?

Simple replace might not work. As then nothing will be pushed on the stack. You might wanna check if the next path is same as the current path and use .push() and .replace() accordingly.

Yokomon commented 3 years ago

Yes that's my point exactly. A condition should be met before .push() or .replace(), well this should be handle on history.

guidobouman commented 3 years ago

Submitted a PR to backport this from the 6 beta to 5.x: #7864

amatiash commented 3 years ago
import React from 'react'
import {Router} from 'react-router-dom'
import {createBrowserHistory} from 'history'

// ----------------------------------------------------

const history = createBrowserHistory()

// ----------------------------------------------------

let currentLocation = window.location.pathname + window.location.search + window.location.hash

history.block((location, action) => {
    const nextLocation   = location.pathname + location.search + location.hash
    const isSameLocation = currentLocation === nextLocation

    if(action === 'PUSH' && isSameLocation)
        return false

    currentLocation = nextLocation
})

// ----------------------------------------------------

const Routes = () => (
    <Router history={history}>
        {/*...*/}
    </Router>
)