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): route lifecycle ordering #27261

Merged
merged 5 commits into from
Oct 8, 2020
Merged

fix(gatsby): route lifecycle ordering #27261

merged 5 commits into from
Oct 8, 2020

Conversation

WillMayger
Copy link
Contributor

@WillMayger WillMayger commented Oct 3, 2020

Description

Rendering is occurring before onPreRouteUpdate when it should be after.

Please see Issue #26608 for a full description.

Related Issues

Fixes #26608

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Oct 3, 2020
@WillMayger WillMayger changed the title [#26608] route lifecycle ordering Closes [#26608] route lifecycle ordering Oct 3, 2020
@WillMayger WillMayger marked this pull request as ready for review October 3, 2020 15:04
@LekoArts LekoArts changed the title Closes [#26608] route lifecycle ordering fix(gatsby): route lifecycle ordering Oct 5, 2020
@LekoArts LekoArts added hacktoberfest-accepted status: needs core review Currently awaiting review from Core team member topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Oct 5, 2020
@wardpeet wardpeet self-assigned this Oct 8, 2020
Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the tests! This is amazing!

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Oct 8, 2020
@gatsbybot gatsbybot merged commit b64f89e into gatsbyjs:master Oct 8, 2020
@y0hnn
Copy link

y0hnn commented Oct 8, 2020

Hey ! This completely breaks plugins like https://transitionlink.tylerbarnes.ca/docs/anilink/

@adonig
Copy link

adonig commented Oct 9, 2020

This might cause the following issue: #27351

@@ -222,6 +216,12 @@ class RouteUpdates extends React.Component {
return false
}

componentDidUpdate(prevProps) {
if (this.props.location.pathname !== prevProps.location.pathname) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this break when other than pathname parts of location are changing? I.e. search, as a result causing problems like #27261

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot! but I do find it strange how this can cause problems like this because the comparison has not changed.

I removed getSnapshotBeforeUpdate which returned a boolean based on the same condition this.props.location.pathname !== prevProps.location.pathname.

That boolean was then passed into componentDidUpdate as the third parameter which is what was being used as the condition just like what I am doing here but without the third param.

So I am not sure I understand why this wasn't an issue before. Regardless - I am happy to investigate this 👍

@WillMayger
Copy link
Contributor Author

WillMayger commented Oct 22, 2020

Fix for location effect triggering here: #27594 & Published in gatsby@2.24.85

pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
* fixing ordering issue by using shouldComponentUpdate

* creating cypress command to check order of actions called

* enabling second page to be able to log lifecycle actions

* writing e2e test to ensure the issue has been fixed and will not return

* removing un-needed findIndex
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes status: needs core review Currently awaiting review from Core team member topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onPreRouteUpdate is not called in correct order rerender and onRouteUpdate
7 participants