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

fix(gatsby): Fix argument order for onPreRouteUpdate #30339

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

StephenCleary
Copy link
Contributor

@StephenCleary StephenCleary commented Mar 18, 2021

This PR is completely untested; I wrote it in the GitHub UI.

Description

The argument for shouldComponentUpdate is the next props, not the previous props, so onPreRouteUpdate was getting called with arguments in the reverse order.

Documentation

https://www.gatsbyjs.com/docs/reference/config-files/gatsby-browser/#onPreRouteUpdate

No doc update necessary; this PR brings the behavior in line with the existing docs.

Related Issues

Fixes #34298

The argument for `shouldComponentUpdate` is the *next* props, not the *previous* props, so `onPreRouteUpdate` was getting called with arguments in the reverse order.
@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 18, 2021
@StephenCleary StephenCleary changed the title Fix argument order for onPreRouteUpdate fix(gatsby) Fix argument order for onPreRouteUpdate Mar 20, 2021
@StephenCleary StephenCleary changed the title fix(gatsby) Fix argument order for onPreRouteUpdate fix(gatsby): Fix argument order for onPreRouteUpdate Mar 20, 2021
@StephenCleary
Copy link
Contributor Author

This has been broken since 2020-10-08 with release gatsby@2.24.70.

@vladar
Copy link
Contributor

vladar commented Mar 26, 2021

This makes sense to me in general.

But can you provide an example project and steps to reproduce the issue? We need to verify both that the issue exists and that this PR fixes it.

@vladar vladar added topic: reach/router and navigation status: needs reproduction This issue needs a simplified reproduction of the bug for further troubleshooting. and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 26, 2021
@StephenCleary
Copy link
Contributor Author

StephenCleary commented Mar 27, 2021

Here ya go. There's one commit with the repro and steps are in the README.

@StephenCleary
Copy link
Contributor Author

Could someone remove the "needs reproduction" tag?

@LekoArts LekoArts added topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation) and removed topic: reach/router and navigation labels May 28, 2021
@dko-slapdash
Copy link

dko-slapdash commented Jul 9, 2021

@vladar I've just spent 2 hours debugging this: the arguments indeed come in the wrong order where location is actually the OLD location and prevLocation is the new location.

The problem only exists for onPreRouteUpdate. The order is correct in onRouteUpdate.

@LekoArts
Copy link
Contributor

LekoArts commented Aug 8, 2022

Hey! Thanks so much for opening this pull request!

Sadly this PR got stale and didn't have any activity for some time. We're trying to do better with PR reviews! To get a better overview of all actionable PRs we're going through all open PRs and triage them. Since we won't be able to do everything and adding new features always means added maintenance burden, we have to be more picky about what's beneficial for the average user and the project itself longterm.

Having said all this, we dropped the ball on this PR and we're sorry about not responding to it earlier. As said above, we want to do better in the future but here we should have communicated earlier what will happen with the PR.

If you or someone else is still hitting this on Gatsby 4, please open a bug report at https://github.com/gatsbyjs/gatsby/issues/new?assignees=&labels=type%3A+bug&template=BUG_REPORT.yml and provide a minimal reproduction. Thanks!

@LekoArts LekoArts closed this Aug 8, 2022
@StephenCleary
Copy link
Contributor Author

Also see #34298

@LekoArts LekoArts removed the status: needs reproduction This issue needs a simplified reproduction of the bug for further troubleshooting. label Apr 12, 2023
@LekoArts LekoArts changed the title fix(gatsby): Fix argument order for onPreRouteUpdate fix(gatsby): Fix argument order for onPreRouteUpdate Apr 12, 2023
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the slow merge on this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: frontend Relates to frontend issues (e.g. reach/router, gatsby-link, page-loading, asset-loading, navigation)
Projects
None yet
6 participants