onPreRouteUpdate is not called in correct order rerender and onRouteUpdate #26608

codepunkt · 2020-08-23T22:13:32Z

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 · 2020-08-25T16:25:19Z

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 · 2020-08-26T15:16:59Z

Nope, just wanted to let you know 👍

github-actions · 2020-09-16T00:12:33Z

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 · 2020-09-16T18:39:42Z

Keep open, seems problematic.

wardpeet · 2020-09-18T08:32:56Z

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.

gatsby/packages/gatsby/cache-dir/navigation.js

Lines 200 to 233 in 55052ef

    
           class RouteUpdates extends React.Component { 
        
             constructor(props) { 
        
               super(props) 
        
               onPreRouteUpdate(props.location, null) 
        
             } 
        
             componentDidMount() { 
        
               onRouteUpdate(this.props.location, null) 
        
             } 
        
             componentDidUpdate(prevProps, prevState, shouldFireRouteUpdate) { 
        
               if (shouldFireRouteUpdate) { 
        
                 onRouteUpdate(this.props.location, prevProps.location) 
        
               } 
        
             } 
        
             getSnapshotBeforeUpdate(prevProps) { 
        
               if (this.props.location.pathname !== prevProps.location.pathname) { 
        
                 onPreRouteUpdate(this.props.location, prevProps.location) 
        
                 return true 
        
               } 
        
               return false 
        
             } 
        
             render() { 
        
               return ( 
        
                 <React.Fragment> 
        
                   {this.props.children} 
        
                   <RouteAnnouncer location={location} /> 
        
                 </React.Fragment> 
        
               ) 
        
             } 
        
           }

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 · 2020-09-21T07:14:52Z

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 · 2020-10-01T06:48:59Z

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!

codepunkt added the type: bug label Aug 23, 2020

gatsbot bot added the status: triage needed label Aug 23, 2020

blainekasten removed the status: triage needed label Aug 25, 2020

github-actions bot added the stale? label Sep 16, 2020

github-actions bot removed the stale? label Sep 17, 2020

wardpeet added the good first issue label Sep 18, 2020

LekoArts added the help wanted label Sep 18, 2020

wardpeet added Hacktoberfest status: confirmed labels Oct 1, 2020

Sep	OCT	Nov
	01
2019	2020	2021

gatsbyjs / gatsby

onPreRouteUpdate is not called in correct order rerender and onRouteUpdate #26608

onPreRouteUpdate is not called in correct order rerender and onRouteUpdate #26608

codepunkt commented Aug 23, 2020

blainekasten commented Aug 25, 2020

codepunkt commented Aug 26, 2020

github-actions bot commented Sep 16, 2020

codepunkt commented Sep 16, 2020

wardpeet commented Sep 18, 2020

codepunkt commented Sep 21, 2020 •

edited

wardpeet commented Oct 1, 2020

gatsbyjs / gatsby

Join GitHub today

onPreRouteUpdate is not called in correct order rerender and onRouteUpdate #26608

onPreRouteUpdate is not called in correct order rerender and onRouteUpdate #26608

Comments

codepunkt commented Aug 23, 2020

Description

Steps to reproduce

Expected result

Actual result

Environment

blainekasten commented Aug 25, 2020

codepunkt commented Aug 26, 2020

github-actions bot commented Sep 16, 2020

codepunkt commented Sep 16, 2020

wardpeet commented Sep 18, 2020

codepunkt commented Sep 21, 2020 • edited

wardpeet commented Oct 1, 2020

codepunkt commented Sep 21, 2020 •

edited