gatsby icon indicating copy to clipboard operation
gatsby copied to clipboard

gatsby-browser.js onPreRouteUpdate passed previous location as location, not the location that is navigated towards.

Open jjang16 opened this issue 3 years ago • 9 comments

Preliminary Checks

  • [X] This issue is not a duplicate. Before opening a new issue, please search existing issues: https://github.com/gatsbyjs/gatsby/issues
  • [X] This issue is not a question, feature request, RFC, or anything other than a bug report. Please post those things in GitHub Discussions: https://github.com/gatsbyjs/gatsby/discussions

Summary

https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#onPreRouteUpdate This documentation says onPreRouteUpdate passes the location we're headed to, but in fact it's passing the previous location. Either the documentation or the implementation has to be fixed.

Steps to Resolve this Issue

  1. implement gatsby-browser.js onPreRouteUpdate, log the location parameter that's passed
  2. add a <Link />
  3. click and see what it prints! ...

jjang16 avatar Dec 20 '21 12:12 jjang16

Gatsby version : 3.14.0


export const onPreRouteUpdate = ({ location, prevLocation } ) => {
/*
  prevLocation passes the "nextLocation"
  and location passes the "prevLocation"
*/
}


It seems location and prevLocation are opposite It's a bug not a documentation error! It's my mistake to open as a documentation issue :(

jjang16 avatar Dec 20 '21 12:12 jjang16

You can close this issue and open a new issue with bug label : )

iChenLei avatar Dec 20 '21 13:12 iChenLei

Hi!

Sorry to hear you're running into an issue. To help us best begin debugging the underlying cause, it is incredibly helpful if you're able to create a minimal reproduction. This is a simplified example of the issue that makes it clear and obvious what the issue is and how we can begin to debug it.

If you're up for it, we'd very much appreciate if you could provide a minimal reproduction and we'll be able to take another look.

Thanks for using Gatsby! 💜

graysonhicks avatar Dec 21 '21 00:12 graysonhicks

Hey this still seems to be an issue, even with the latest version of Gatsby - v4.6.2

https://codesandbox.io/s/blissful-lichterman-gf8xl

image

The first set of logs (onPreRouteUpdate) is incorrect, showing us navigating from the /using-typescript to the root /; however the second set of logs (onRouteUpdate) is correct.

RBrNx avatar Feb 07 '22 20:02 RBrNx

Same thing. I'm in Gatsby 3 still, but the two variables are switched!!!

export const onPreRouteUpdate = ({ location, prevLocation }) => {
  // location = previous location
  // prevLocation = next location

doublejosh avatar May 06 '22 19:05 doublejosh

Is there a bug report for this? @jjang16 Otherwise, it should not be closed.

doublejosh avatar May 06 '22 19:05 doublejosh

@doublejosh I am reopening. Here is a reproduction:

https://codesandbox.io/s/ecstatic-hamilton-o026sk?file=/gatsby-browser.js

Steps to Reproduce

  • go to sandbox
  • when sandbox is booted, go to homepage of the site in sandbox tab
  • open the console
  • click the Using Typescript link

Expected Result

Gatsby started to change location to /using-typescript/ 
Gatsby started to change location from /

Actual Result

Gatsby started to change location to /
Gatsby started to change location from /using-typescript/ 

Note the paths are switched. The Gatsby source for this hook is here: https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby/cache-dir/navigation.js#L227

It looks correct to me, so not sure what is happening, possibly a React state issue?

graysonhicks avatar May 07 '22 05:05 graysonhicks

@graysonhicks

It looks correct to me

As noted in my PR, the argument for shouldComponentUpdate is the next props, not the previous props. So the argument variable named prevProps should actually be called nextProps.

The variable name has been misleading since this commit which changed the trigger from getSnapshotBeforeUpdate (which takes a prevProps) to shouldComponentUpdate (which takes a nextProps).

StephenCleary avatar Aug 08 '22 11:08 StephenCleary

Nice catch @StephenCleary !

graysonhicks avatar Aug 09 '22 13:08 graysonhicks

Ok so summarizing what has happened here:

  1. One of Gatsby's Browser API functions is blatantly incorrect, and has been since v2.
  2. A community member, @StephenCleary found the exact part of the code that was wrong (it's a logic error on the part of the Gatsby framework) and actually opened a PR to fix the issue back in March 2021 (2 years ago now)
  3. The Gatsby team got busy and completely ignored this for a while
  4. A Gatsby team member (@graysonhicks) linked/created a minimal reproduction showing this bug in May 2022
  5. In August 2022 a Gatsby team member (@LekoArts) recognized that they had dropped the ball on the PR, said that they would be triaging open PRs, said something off-hand about adding new features, and then closed the PR without reviewing or accepting the changes
  6. In response, the original PR author commented here and linked their PR with a really clear explanation (again) of what was causing this bug.
  7. Inexplicably, a Gatsby team member (@graysonhicks) said "nice catch" but did literally nothing to fix this issue

To date, Gatsby broke one of their base APIs and left it broken for 2 years and 5 months (and counting) even though one of their users posted a PR fixing the issue, explaining the error, and following up calmly and respectfully several times. This fix could be as simple as renaming a single prop, or updating some docs that are entirely incorrect.

I have been a consistent and avid user of Gatsby on client and personal projects for over 3 years now, but it's stuff like this that makes it harder and harder for me to recommend as a good framework.

panzacoder avatar Mar 27 '23 21:03 panzacoder

@panzacoder I've reopened the PR.

graysonhicks avatar Apr 03 '23 13:04 graysonhicks