[v2] [cache-dir] Fix navigation after reload #7812

vtenfys · 2018-09-02T19:14:26Z

We added popstate listeners to app.js and production-app.js which load the resources / render the page when back/forward is pressed.

onPreRouteUpdate and onRouteUpdate aren't currently being called when navigating with back/forward so this will need to be fixed later in order for plugins to work properly.

Note: wait until #7936 is merged to master before merging this PR, so we can properly test CI beforehand. ~~Just merged master~~ oops I got mixed up with a different one of DSchau's PRs which was merged

Fixes #7261

pieh · 2018-09-02T20:21:18Z

Some context on commented out on(Pre)RouteUpdate calls: we currently don't call onRouteUpdate when using history. I asked to comment them out, because with that they will run only in very specific scenario - hitting "back" after reloading window. This should be fixed in separately.

KyleAMathews · 2018-09-03T15:52:55Z

With @pieh's PR improving our Cypress setup landing, it'd be nice for this PR to write a test for the reload and back button behavior which fails without the changes made here but then which the changes here do fix .

…back-after-reload

vtenfys · 2018-09-04T10:09:17Z

@KyleAMathews I've just added the new test - waitForRouteChange() doesn't seem to work with the 404 development page (it resolves after the URL changes but before the 404 appears) so I've gone for the approach of detecting whether the image wrapper element exists, since Cypress will auto-wait for this. I've tested before and after the changes to check it fails and passes correctly.

pieh · 2018-09-04T10:48:13Z

@davidbailey00 waitForRouteChange problem is related to problems with navigating through history and firing onRouteUpdate - it simply doesn't detect that route has changed in that case and only remember when DOM was updated after reload (before hitting back). Also this currently fail on travis - travis seems much slower, so that's why we added waitForRouteChange in the first place, because cypress timeouts were not reliable

vtenfys · 2018-09-04T14:21:22Z

@pieh With the Travis tests, currently we're not using gatsby-dev to copy the latest changes so it's expected to fail at the current stage (because it's using the latest published version). I'm pretty sure that it should pass once the update has been published, regardless of the speed of Travis, because Cypress will automatically wait until the div[to='/${post1.id}/'] element exists - the problem with earlier tests is that some elements did exist but Cypress clicked them before event handlers were setup, but there's no such problem here because it's just checking for the existence.

pieh · 2018-09-04T14:25:22Z

It will wait ~4s, which sometimes proved to be not enough on Travis - when you test this locally with cypress open - try throthling cpu (6x slowodown) and run test multiple times - it proved to quite ok mimic travis slowness. But defenitelly good point about gatsby-dev (which I sometimes forget :/)

vtenfys · 2018-09-04T14:31:23Z

@pieh In that case perhaps we can override Cypress's default timeout with something like 10 seconds - but I think it's unlikely Travis will be that slow in most cases, given that only two of the tests took longer than 1 second:

KyleAMathews · 2018-09-04T16:44:17Z

The plan is to build code & run gatsby-dev for tests but yeah, that makes it more difficult now.

@davidbailey00 could you add tests to here as well? https://github.com/gatsbyjs/gatsby/tree/master/integration-tests/production-runtime

Gatsbygram is running the develop server.

pieh · 2018-09-05T12:45:40Z

Hey, when I was diving deep into PageRenderer.js for onRouteUpdate fixes for history I noticed we have code there to handle exactly this

gatsby/packages/gatsby/cache-dir/page-renderer.js

Line 72 in 0c032cf

loader.getResourcesForPathnameSync(pathName, pageResources => {

but this isn't correct and we didn't catch this before - this line should not be sync and use then:

-loader.getResourcesForPathnameSync(pathName, pageResources => { 
+loader.getResourcesForPathname(pathName).then(pageResources => {

I think changing this and reverting your changes from production-app.js and app.js should be correct way to fix this. And we still definitely want cypress test you added (which I would later modify a little to use onRouteUpdate when I fix that)

vtenfys · 2018-09-05T13:57:52Z

@pieh Nice find, I'll try changing this later tomorrow (just sorting out college stuff now).

…derer

…rate PR

pieh · 2018-09-05T22:00:12Z

Change I commented above fixed this for production, but development has a little different setup and we are getting stuck in JsonStore component before we even get to PageRenderer so I kept development change for now - ultimately I think we could move ad-hoc resources loading from PageRenderer somewhere higher in component tree - I'll experiment now for a bit, but won't spend too much time on it

vtenfys · 2018-09-06T13:35:35Z

@pieh Sounds good, thanks for making the changes! I've got my initial college homework out the way now so I'll try adding the tests requested by Kyle later on.

…s can be removed

this is mostly covered by navigation code, but in case of using history.back / forward functions, browser will change URL immediately and it's not handled by navigation code then.

…ee state of DOM

KyleAMathews · 2018-09-07T01:21:50Z

Just thinking. We should publish our Cypress plugin to npm as people will want to use it when testing their gatsby sites as well. Plus it could lead to more useful Cypress plugins being added.

…at for publicly exported PageRenderer

pieh · 2018-09-07T01:27:44Z

Just thinking. We should publish our Cypress plugin to npm as people will want to use it when testing their gatsby sites as well. Plus it could lead to more useful Cypress plugins being added.

I agree, I just didn't want to focus on finding proper way of packaging cypress extension + that API is very fresh and I'm not sure if it will stick and if we won't have to change it - I guess it would work if we release it as 0.x version when nothing is guaranteed ;)

pieh · 2018-09-07T02:24:13Z

Feeling pretty good about.

Here is some manual testing I did to ensure on(pre)RouteUpdate and resolving and resetting rout change promise is done in correct order and at correct times:

I disabled prefetching resources by doing early returns in hovering and enqueue functions here (

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

Lines 182 to 187 in 4efe8f9

    
           hovering: rawPath => { 
        
             const path = stripPrefix(rawPath, __PATH_PREFIX__) 
        
             queue.getResourcesForPathname(path) 
        
           }, 
        
           enqueue: rawPath => { 
        
             const path = stripPrefix(rawPath, __PATH_PREFIX__)

) and verified by checking network tab (and additionally checking web socket frames in development for data)

my testing site was basicly gatsby-starter-blog with additional gatsby-browser.js that console.log on(Pre)RouteUpdate that checks actual DOM element with document.getElementById (titleElementInDom)
my testing methology was just loading index page in browser and then issuing following commands in same order for production and development:
- window.___navigate('/hi-folks/') (go to page that exists)
- window.___navigate('/this-will-be-404') (go to page that doesn't exist)
- window.location.reload() (reload to start actually testing main thing that this PR should do :D)
- window.history.back() (go back from 404 page to existing page)
- window.location.reload()
- window.history.forward() (go forward from existing page to 404)

Production:

Console output: screenshot (colorful yay)

Console output: text

Navigated to http://localhost:9000/
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resetRouteChangePromise
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "IndexPage", pathname: "/"}
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onRouteUpdate {titleElementInDOM: "IndexPage", pathname: "/"}
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resolveRouteChangePromise
window.___navigate('/hi-folks/')
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resetRouteChangePromise
undefined
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "IndexPage", pathname: "/hi-folks/"}
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resolveRouteChangePromise
window.___navigate('/this-will-be-404')
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resetRouteChangePromise
VM284 app-e36d6dfe3bf7e083bcc8.js:1 A page wasn't found for "/this-will-be-404"
undefined
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/404.html"}
VM284 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
VM284 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resolveRouteChangePromise
window.location.reload()
undefined
Navigated to http://localhost:9000/this-will-be-404
VM393 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resetRouteChangePromise
VM393 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
VM393 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
VM393 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resolveRouteChangePromise
window.history.back()
undefined
VM393 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resetRouteChangePromise
VM393 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/hi-folks/"}
VM393 app-e36d6dfe3bf7e083bcc8.js:6 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
VM393 app-e36d6dfe3bf7e083bcc8.js:1 [wait-for-route-change] resolveRouteChangePromise
window.location.reload()
undefined
Navigated to http://localhost:9000/hi-folks/
wait-for-route-change.js:3 [wait-for-route-change] resetRouteChangePromise
gatsby-browser.js:11 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
gatsby-browser.js:3 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
wait-for-route-change.js:12 [wait-for-route-change] resolveRouteChangePromise
window.history.forward()
undefined
wait-for-route-change.js:3 [wait-for-route-change] resetRouteChangePromise
gatsby-browser.js:11 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/404.html"}
gatsby-browser.js:3 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
wait-for-route-change.js:12 [wait-for-route-change] resolveRouteChangePromise

Development:

Console output: screenshot (colorful yay)

Console output: text

Navigated to http://localhost:8000/
VM2352 wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
VM2193 __webpack_hmr&reload=true&overlay=false:92 [HMR] connected
VM2308 gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: null, pathname: "/"}
VM2308 gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "IndexPage", pathname: "/"}
VM2352 wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.___navigate('/hi-folks/')
VM2352 wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
undefined
VM2308 gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "IndexPage", pathname: "/hi-folks/"}
VM2308 gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
VM2352 wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.___navigate('/this-will-be-404')
VM2352 wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
VM2309 loader.js:340 A page wasn't found for "/this-will-be-404"
undefined
VM2308 gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/404.html"}
VM2308 gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
VM2352 wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.location.reload()
undefined
Navigated to http://localhost:8000/this-will-be-404
VM2787 wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
VM2744 loader.js:340 A page wasn't found for "/this-will-be-404"
VM2628 __webpack_hmr&reload=true&overlay=false:92 [HMR] connected
VM2743 gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: null, pathname: "/404.html"}
VM2743 gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
VM2787 wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.history.back()
undefined
VM2787 wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
VM2743 gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/hi-folks/"}
VM2743 gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
VM2787 wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.location.reload()
undefined
Navigated to http://localhost:8000/hi-folks/
Users/misiek/dev/my-blog/.cache/wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
Users/misiek/dev/my-blog/node_modules/webpack-hot-middleware/client.js?path=http:/localhost:8000/__webpack_hmr&reload=true&overlay=false:92 [HMR] connected
Users/misiek/dev/my-blog/gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: null, pathname: "/hi-folks/"}
Users/misiek/dev/my-blog/gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/hi-folks/"}
Users/misiek/dev/my-blog/.cache/wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise
window.history.forward()
undefined
Users/misiek/dev/my-blog/.cache/wait-for-route-change.js:18 [wait-for-route-change] resetRouteChangePromise
Users/misiek/dev/my-blog/gatsby-browser.js:13 [gatsby-browser] onPreRouteUpdate {titleElementInDOM: "Post: New Beginnings", pathname: "/404.html"}
Users/misiek/dev/my-blog/gatsby-browser.js:4 [gatsby-browser] onRouteUpdate {titleElementInDOM: "NOT FOUND", pathname: "/404.html"}
Users/misiek/dev/my-blog/.cache/wait-for-route-change.js:31 [wait-for-route-change] resolveRouteChangePromise

Difference between dev and prod is that DOM element exists in production for onPreRouteUpdate after initial load and reloads (because of static html :) )

pieh · 2018-09-07T13:03:21Z

note to self: consider moving ScrollContext down in component tree as scroll position can reset now before new page have resources (f.e. after reloading page and going back)

pieh · 2018-09-07T17:10:09Z

packages/gatsby/cache-dir/ensure-resources.js

+  }
+
+  static getDerivedStateFromProps({ pageResources, location }, prevState) {
+    if (prevState.location.pathname !== location.pathname) {


need to make shallow compare - location changes doesnt mean pathname changes

pieh · 2018-09-07T17:14:32Z

packages/gatsby/cache-dir/ensure-resources.js

+    // Check if location has changed on a page using internal routing
+    // via matchPath configuration.
+    if (
+      this.props.location.key !== nextProps.location.key &&


verify key is there in @reach/router

pieh · 2018-09-07T17:15:41Z

packages/gatsby/cache-dir/ensure-resources.js

+
+      return null
+    } else if (
+      process.env.NODE_ENV !== `production` &&


verify if this ever happen

…back-after-reload

vtenfys · 2018-09-09T18:33:31Z

The Cypress production test I added is failing 80% of the time with Electron 59, but working perfectly every time on the latest Chrome (which we can't use with CI). I spent a while this morning trying to figure out the problem but couldn't find it, and then had to do some other things - I'll have another look now.

vtenfys · 2018-09-09T20:43:52Z

It works! Travis still fails at the moment because we don't use gatsby-dev before testing, but #7936 will fix this (we should merge master to this PR, once #7936 is merged to master, to check it works).

KyleAMathews · 2018-09-10T01:02:47Z

Great work @davidbailey00! Totally fine merging this w/ manual local testing until #7936 is working. I assume @pieh will get this in before I'm awake in the morning but if not, I'll test and merge.

pieh · 2018-09-10T14:13:00Z

We had conversation with @davidbailey00 and there is one more issue that need fixing here - in production sync loader.getPage things can fail to get page if we call it before we fetch pages metadata - which is what I think was causing problems on cypress/electron (and not really some bug there). So i currently work on that

…ome dead code

pieh · 2018-09-10T14:50:14Z

still TO-DO - move scroll context component to PageRenderer

This reverts commit 6d7cbf5.

…back-after-reload

David Bailey added 2 commits September 2, 2018 19:48

Fix navigation after reloading

5a2c955

Add commented apiRunner calls

3d764bf

David Bailey added 2 commits September 4, 2018 10:19

Merge branch 'master' of https://github.com/gatsbyjs/gatsby into fix-…

11e54dc

…back-after-reload

Add new test for back after reload and upgrade packages

5aafac0

pieh added 3 commits September 5, 2018 23:50

fix fetching resources in page-renderer

b560d8f

revert changes to production-app - this is handled by fix in page-ren…

a79e6c6

…derer

remove commented out routeUpdate api calls - it will be fixed in sepa…

8a9cca7

…rate PR

pieh and others added 9 commits September 6, 2018 16:12

revert changes to app.js

0ee31ee

split page renderer into 2 componets

fce3c71

update cypress test to go back to home page, so next test start fresh

994af2b

Add Cypress production test

a537a36

this isn't doing anything - json is no longer in pageResources in dev

cd4dd61

we no longer transition to page when it takes long time to load - thi…

135e2aa

…s can be removed

keep rendering old page while waiting for resources for new page

3771e44

this is mostly covered by navigation code, but in case of using history.back / forward functions, browser will change URL immediately and it's not handled by navigation code then.

move firing on(Pre)RouteUpdate APIs to pageRenderer - it will guarant…

56605d5

…ee state of DOM

no need to destructure these

338aa74

only fire on(Pre)RouteUpdate APIs for main PageRenderer - don't do th…

43a3a5f

…at for publicly exported PageRenderer

reset route change promise when going back or forward in history

a7c9904

pieh reviewed Sep 7, 2018

View reviewed changes

Merge branch 'master' of https://github.com/gatsbyjs/gatsby into fix-…

83ae004

…back-after-reload

Use Chrome for Cypress tests

fdf6954

David Bailey added 2 commits September 10, 2018 13:38

Remove waitForRouteChange after reloading, and test the URL

205d70d

Undo using Chrome on CI

6d7cbf5

pieh added 3 commits September 10, 2018 16:33

wait for full load in cypress

61e595e

handle loading 404 resources in loader.js

e96ff5b

update location changes to page even pathname didn't change, remove s…

c1d4797

…ome dead code

David Bailey and others added 6 commits September 10, 2018 20:17

Revert "Undo using Chrome on CI"

0ec183a

This reverts commit 6d7cbf5.

Merge branch 'master' of https://github.com/gatsbyjs/gatsby into fix-…

0883346

…back-after-reload

Import Cypress commands from package

5edd24b

Merge branch 'master' of https://github.com/gatsbyjs/gatsby into fix-…

48c16ed

…back-after-reload

Merge remote-tracking branch 'origin/master' into fix-back-after-reload

1837164

revert gatsbygram package dependency version changes

3d97465

pieh approved these changes Sep 13, 2018

View reviewed changes

pieh merged commit 224f6a8 into gatsbyjs:master Sep 13, 2018

vtenfys mentioned this pull request Sep 15, 2018

[v2] Navigating to previously visited pages with <Link> retains scroll position #7454

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v2] [cache-dir] Fix navigation after reload #7812

[v2] [cache-dir] Fix navigation after reload #7812

vtenfys commented Sep 2, 2018 •

edited

Loading

pieh commented Sep 2, 2018

KyleAMathews commented Sep 3, 2018

vtenfys commented Sep 4, 2018

pieh commented Sep 4, 2018

vtenfys commented Sep 4, 2018

pieh commented Sep 4, 2018

vtenfys commented Sep 4, 2018

KyleAMathews commented Sep 4, 2018

pieh commented Sep 5, 2018

vtenfys commented Sep 5, 2018

pieh commented Sep 5, 2018

vtenfys commented Sep 6, 2018

KyleAMathews commented Sep 7, 2018

pieh commented Sep 7, 2018

pieh commented Sep 7, 2018

pieh commented Sep 7, 2018

pieh Sep 7, 2018

pieh Sep 7, 2018

pieh Sep 7, 2018

vtenfys commented Sep 9, 2018

vtenfys commented Sep 9, 2018

KyleAMathews commented Sep 10, 2018

pieh commented Sep 10, 2018

pieh commented Sep 10, 2018

[v2] [cache-dir] Fix navigation after reload #7812

[v2] [cache-dir] Fix navigation after reload #7812

Conversation

vtenfys commented Sep 2, 2018 • edited Loading

pieh commented Sep 2, 2018

KyleAMathews commented Sep 3, 2018

vtenfys commented Sep 4, 2018

pieh commented Sep 4, 2018

vtenfys commented Sep 4, 2018

pieh commented Sep 4, 2018

vtenfys commented Sep 4, 2018

KyleAMathews commented Sep 4, 2018

pieh commented Sep 5, 2018

vtenfys commented Sep 5, 2018

pieh commented Sep 5, 2018

vtenfys commented Sep 6, 2018

KyleAMathews commented Sep 7, 2018

pieh commented Sep 7, 2018

pieh commented Sep 7, 2018

Production:

Development:

pieh commented Sep 7, 2018

pieh Sep 7, 2018

Choose a reason for hiding this comment

pieh Sep 7, 2018

Choose a reason for hiding this comment

pieh Sep 7, 2018

Choose a reason for hiding this comment

vtenfys commented Sep 9, 2018

vtenfys commented Sep 9, 2018

KyleAMathews commented Sep 10, 2018

pieh commented Sep 10, 2018

pieh commented Sep 10, 2018

vtenfys commented Sep 2, 2018 •

edited

Loading