Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Link should perform a replace when navigating to the current page #22124

Closed
3nvi opened this issue Mar 10, 2020 · 8 comments · Fixed by #23414
Closed

Link should perform a replace when navigating to the current page #22124

3nvi opened this issue Mar 10, 2020 · 8 comments · Fixed by #23414
Labels
stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby

Comments

@3nvi
Copy link

3nvi commented Mar 10, 2020

Description

When rendering a <Link> from gatsby-link which points to the current location, then each click pushes a new entry into the history stack. Thus, If I click a link (pointing to the page that I'm on) multiple times, then I require the same amount of browser "back button" presses, in order to leave my current page.

Natively, browsers implement a history "replace" and not a "push" when the anchor tag points to the current URL. I feel that gatsby's Link should to the same.

There was also a related issue in reach router

Steps to reproduce

Start by creating a new site

gatsby new test-site
cd test-site
gatsby develop

Then:

  1. Open a new browser tab
  2. Navigate to localhost:8000
  3. Click on the Gatsby Default Starter multiple times
  4. Press the browser back button

Expected result

You would expect to go back to your tab's initial page

Actual result

Nothing happens until you press back a lot of times (as many as your Gatsby Default Starter clicks)

Environment

System:
OS: macOS 10.15.2
CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
Shell: 5.7.1 - /bin/zsh
Binaries:
Node: 10.16.0 - /usr/local/bin/node
Yarn: 1.19.0 - /usr/local/bin/yarn
npm: 6.11.3 - /usr/local/bin/npm
Languages:
Python: 2.7.16 - /usr/bin/python
Browsers:
Chrome: 80.0.3987.132
Firefox: 72.0.2
Safari: 13.0.4
npmPackages:
gatsby: ^2.18.1 => 2.19.19
gatsby-image: ^2.2.33 => 2.2.41
gatsby-plugin-brotli: ^1.3.1 => 1.3.1
gatsby-plugin-manifest: ^2.2.29 => 2.2.41
gatsby-plugin-netlify: ^2.1.33 => 2.1.33
gatsby-plugin-offline: ^3.0.22 => 3.0.35
gatsby-plugin-prefetch-google-fonts: ^1.4.3 => 1.4.3
gatsby-plugin-react-helmet: ^3.1.15 => 3.1.22
gatsby-plugin-sass: ^2.1.23 => 2.1.29
gatsby-plugin-sharp: ^2.3.2 => 2.4.5
gatsby-plugin-sitemap: ^2.2.27 => 2.2.27
gatsby-plugin-typescript: ^2.1.27 => 2.1.27
gatsby-source-filesystem: ^2.1.38 => 2.1.48
gatsby-transformer-sharp: ^2.3.5 => 2.3.14
npmGlobalPackages:
gatsby-cli: 2.10.4

@3nvi 3nvi added the type: bug An issue or pull request relating to a bug in Gatsby label Mar 10, 2020
@pieh
Copy link
Contributor

pieh commented Mar 12, 2020

Ok, so seems like reach/router fixed this only when using reach/router Link components, but we do overwrite this and call navigate in reach/router directly (thus skipping path where checks in reach/router happen).

I'm not exactly sure when replace should happen - do all url parts (domain, pathname, hash, query string) need to match - or just domain and pathname - I think reach checks for all parts now

@3nvi
Copy link
Author

3nvi commented Mar 12, 2020

Exactly, gatsby uses the imperative navigate instead of wrapping the actual component. Since it has been identified as an issue in the underlying navigation library, I feel that Gatsby should adjust as well.

I'm not aware whether removing the onClick and delegating it to @react/router will create any issues though :/

related code

@pieh
Copy link
Contributor

pieh commented Mar 12, 2020

It's not that simple, because our navigation code also handle resource loading (primary reason why we don't use reach/router link components really) before doing actual navigation. Otherwise we would have flashes of content if use reach navigation and only after load resources

@pieh
Copy link
Contributor

pieh commented Mar 12, 2020

We also can't use something like Suspend because minimal react version supported by gatsby@2 doesn't have Suspend support :S (not to mention that data loading with Suspense is not yet stable AFAIK and throwing method to suspend might not be the final API in react (?) )

@pieh
Copy link
Contributor

pieh commented Mar 12, 2020

Maybe solution here would be to move the checks in @reach/router so they are not in Link component, but rather in navigate function. Other options are to re-implement this in gatsby runtime.

I wonder what @blainekasten thinks about this ;)

@3nvi
Copy link
Author

3nvi commented Mar 12, 2020

Gotcha, yeah I didn't dig into the "whys" at all. Understood. Perhaps mimicking the same logic that exists in @reach/router?

Would you accept a PR to add a behavior similar to this?

I know it's not optimal, but it's a solution

@github-actions
Copy link

github-actions bot commented Apr 2, 2020

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 30 days of inactivity. It’s been at least 20 days since the last update here.
If we missed this issue or if you want to keep it open, please reply here. You can also add the label "not stale" to keep this issue open!
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks for being a part of the Gatsby community! 💪💜

@github-actions github-actions bot added the stale? Issue that may be closed soon due to the original author not responding any more. label Apr 2, 2020
@github-actions
Copy link

Hey again!

It’s been 30 days since anything happened on this issue, so our friendly neighborhood robot (that’s me!) is going to close it.
Please keep in mind that I’m only a robot, so if I’ve closed this issue in error, I’m HUMAN_EMOTION_SORRY. Please feel free to reopen this issue or create a new one if you need anything else.
As a friendly reminder: the best way to see this issue, or any other, fixed is to open a Pull Request. Check out gatsby.dev/contribute for more information about opening PRs, triaging issues, and contributing!

Thanks again for being part of the Gatsby community! 💪💜

abarisain added a commit to abarisain/gatsby that referenced this issue Apr 23, 2020
Fixes gatsbyjs#22124

If the developer didn't explicitly set the "replace" prop and we're on
the page that we're about to navigate to, force "replace" to true
when navigating.
This fixes a bug where clicking a link that does not result in
navigating to another page incorrectly
adds an history entry.

Uses the same technique as @reach/router
blainekasten pushed a commit that referenced this issue May 7, 2020
… it (#23414)

Fixes #22124

If the developer didn't explicitly set the "replace" prop and we're on
the page that we're about to navigate to, force "replace" to true
when navigating.
This fixes a bug where clicking a link that does not result in
navigating to another page incorrectly
adds an history entry.

Uses the same technique as @reach/router
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale? Issue that may be closed soon due to the original author not responding any more. type: bug An issue or pull request relating to a bug in Gatsby
Projects
None yet
2 participants