Gatsby: onPreRouteUpdate is not called in correct order rerender and onRouteUpdate

[codepunkt]

Description

I have a onPreRouteUpdate and a onRouteUpdate in gatsby-browser, both log to the console. I also have a layout component that logs on render and wrapped via gatsby-browsers wrapPageElement.

On initial render, the logging order is: onPreRouteUpdate > layout > onRouteUpdate
On subsequent navigations, the logging order is: layout > onPreRouteUpdate > onRouteUpdate

This seems confusing and wrong. If onPreRouteUpdate is supposed to be triggered _before_ a route update, layout cannot possibly be rerendered before that. Ordering seems wrong here.

Steps to reproduce

https://codesandbox.io/s/morning-silence-19rkd?file=/gatsby-browser.js
https://19rkd-8000.sse.codesandbox.io/

Expected result

Logging order should always be: onPreRouteUpdate > layout > onRouteUpdate
This should be the order for both initial render and subsequent navigations.

Actual result

On initial render, the logging order is: onPreRouteUpdate > layout > onRouteUpdate
On subsequent navigations, the logging order is: layout > onPreRouteUpdate > onRouteUpdate

Environment

System:
OS: Linux 4.19 Ubuntu 20.04.1 LTS (Focal Fossa)
CPU: (8) x64 Intel(R) Core(TM) i7-7700K CPU @ 4.20GHz
Shell: 5.8 - /usr/bin/zsh
Binaries:
Node: 12.18.3 - /tmp/yarn--1598220669129-0.9999174876381276/node
Yarn: 1.22.4 - /tmp/yarn--1598220669129-0.9999174876381276/yarn
npm: 6.14.6 - ~/.nvm/versions/node/v12.18.3/bin/npm
Browsers:
Chrome: 84.0.4147.125
npmPackages:
gatsby: ^2.24.48-incbuild-collections.5 => 2.24.48-incbuild-collections.5+598335ebeb
gatsby-image: ^2.4.5 => 2.4.5
gatsby-plugin-linaria: ^2.0.0 => 2.0.0
gatsby-plugin-manifest: ^2.4.9 => 2.4.9
gatsby-plugin-mdx: ^1.2.15 => 1.2.15
gatsby-plugin-offline: ^3.2.7 => 3.2.7
gatsby-plugin-react-helmet: ^3.3.2 => 3.3.2
gatsby-plugin-remove-generator: ^1.0.5 => 1.0.5
gatsby-plugin-sharp: ^2.6.9 => 2.6.9
gatsby-remark-copy-linked-files: ^2.3.12 => 2.3.12
gatsby-remark-shiki-twoslash: ^0.6.1 => 0.6.1
gatsby-source-filesystem: ^2.3.8 => 2.3.8
gatsby-transformer-sharp: ^2.5.3 => 2.5.3

[blainekasten]

This makes sense to me. I'll discuss internally to see if there is a reason we cant make this change. Would you be interested in submitting a PR to fix this?

[codepunkt]

Nope, just wanted to let you know 👍

[github-actions[bot]]

Hiya!

This issue has gone quiet. Spooky quiet. 👻

We get a lot of issues, so we currently close issues after 60 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.
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! 💪💜

[codepunkt]

Keep open, seems problematic.

[wardpeet]

We won't be able to prioritize this change but we're happy to get a pull request for it. Make sure to add tests to test this behavior in one of our e2e tests.

https://github.com/gatsbyjs/gatsby/blob/55052eff7d00cdccb4a8439972b124e10dd06b7d/packages/gatsby/cache-dir/navigation.js#L200-L233

This is a nice graph on why this bug exists:
https://projects.wojtekmaj.pl/react-lifecycle-methods-diagram/

diff --git a/packages/gatsby/cache-dir/navigation.js b/packages/gatsby/cache-dir/navigation.js
index d870bfec6..b50d88d91 100644
--- a/packages/gatsby/cache-dir/navigation.js
+++ b/packages/gatsby/cache-dir/navigation.js
@@ -207,6 +207,14 @@ class RouteUpdates extends React.Component {
     onRouteUpdate(this.props.location, null)
   }

+  shouldComponentUpdate(nextProps) {
+    if (this.props.location.pathname !== nextProps.location.pathname) {
+      onPreRouteUpdate(nextProps.location, this.props.location)
+    }
+
+    return true
+  }
+
   componentDidUpdate(prevProps, prevState, shouldFireRouteUpdate) {
     if (shouldFireRouteUpdate) {
       onRouteUpdate(this.props.location, prevProps.location)
@@ -215,7 +223,6 @@ class RouteUpdates extends React.Component {

   getSnapshotBeforeUpdate(prevProps) {
     if (this.props.location.pathname !== prevProps.location.pathname) {
-      onPreRouteUpdate(this.props.location, prevProps.location)
       return true
     }

[codepunkt]

We won't be able to prioritize this change but we're happy to get a pull request for it. Make sure to add tests to test this behavior in one of our e2e tests.

If you consider this a bug, i would expect you to fix it.
If you consider this working as expected, please close this issue with an official "working as expected" comment.

Your intention is not clear. What does "won't be able to prioritize" mean for you in this context?

[wardpeet]

I'm sorry I feel like I gave enough information for community members to pick it up. We have many things in flight and we have to juggle where we put most of our effort in. I hope you understand and feel free to create a PR!

[clovis1122]

I'll take a look and see if I can submit a PR for this one.

[WillMayger]

👋 Hi everyone, is it alright if I also take a look at this one? Seems pretty interesting!

[clovis1122]

@WillMayger sure thing! I'm still exploring the repo. I don't mind if your fix is merged instead of mine -- will be mostly a learning experience for me, haha.

[WillMayger]

@clovis1122 Ah I see! - thanks for letting me take a look 😄 & I have a PR ready for review at https://github.com/gatsbyjs/gatsby/pull/27261

[clovis1122]

Woah that was fast! Great job, I'll drop and see if I can tackle other issues haha.

[WillMayger]

Thanks 😄! and haha I think it seems faster than it actually was, I started playing around with it right after I asked about it 😅 also @wardpeet had already done a lot of the work!

[wardpeet]

Published in [email protected]