Fix url navigation when window.___history does not exist #2667

efallancy · 2017-10-29T06:37:25Z

This PR relates to #1838 . This would particularly fix the history.push for when window.___history does not exist, especially when no page found and the web host would serve 404.html page. Also, gatsby-link depends on window.___navigateTo and that also depends on window.___history to navigate between pages.

@KyleAMathews I hope this shall be able to fix the problem with navigating in 404 pages.

gatsbybot · 2017-10-29T06:49:01Z

Deploy preview ready!

Built with commit a4feba9

https://deploy-preview-2667--gatsbygram.netlify.com

efallancy · 2017-10-29T06:49:32Z

packages/gatsby/cache-dir/production-app.js

+        if (window.___history) {
+          window.___history.push(pathname)
+        } else {
+          window.location.href = pathname


Hi @KyleAMathews , based on my investigation, gatsby-link uses window.___navigateTo to navigate between pages and window.navigateTo relies on window.___history to navigate. Apparently, when a resource/page not found in the directory, the host will use 404.html to serve page not found and window.___history does not seem to exist. Hence, using window.location.href to navigate between pages.

KyleAMathews · 2017-10-30T22:57:51Z

Nice investigative work!

This doesn't seem like the right fix though — is there any reason we can just ensure our history object is placed on ___window before 404 handling happens?

KyleAMathews · 2017-10-30T23:16:32Z

Deploy preview failed.

Built with commit 5fa690e

https://app.netlify.com/sites/using-glamor/deploys/59f7ae20a6188f5421feeb84

efallancy · 2017-10-31T00:24:13Z

I thought about it too before thought. Just that I have not dig into finding a way to ensure that the window.___history persist. I'll have a look at it later. Was thinking if probably when scripts being loaded, we can ensure or potentially inject it if window.___history not found? If we can get window.history, I sure believe we can get window.___history as well. 😄

efallancy · 2017-10-31T00:46:35Z

@KyleAMathews i just skimmed through just now and was wondering would it be fine to just attach/assign the history immediately right after line 24.

Since window.___emitter and window.___loader assigned at the top, that could probably be the reason why it can still be access even when it hits to 404. Also, based on current implementation, the attachment of history is done in router (attachToHistory(routeProps.history)), as well as checking. Any thoughts? 😄

…c/fix-404-navigate-page

…fallancy/gatsby into topic/fix-404-navigate-page

…ing page not found

KyleAMathews · 2017-10-31T02:54:47Z

Went to https://deploy-preview-2667--gatsbyjs.netlify.com/sdfsdf and still couldn't click to another page so looks like something is still broken.

efallancy · 2017-10-31T03:01:24Z

@KyleAMathews good finding. Was waiting for the notification of review app ready but you manage to got there early 😅

Alright, I'll investigate further later. Sorry about that. Thanks for confirming 🙇

…ge found during production and ensure a safe guard in component renderer checking

efallancy · 2017-10-31T17:09:46Z

packages/gatsby/cache-dir/production-app.js

@@ -163,7 +163,8 @@ apiRunnerAsync(`onClientEntry`).then(() => {
                    })
                  } else {
                    return createElement(ComponentRenderer, {
-                      location: { page: true, pathname: `/404.html` },
+                      page: true,


Changed it into this way instead, since ComponentRenderer is accepting and accessing page props instead of getting it from location props.

efallancy · 2017-10-31T17:11:03Z

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

@@ -61,7 +61,10 @@ class ComponentRenderer extends React.Component {
    // This is only useful on delayed transitions as the page will get rendered
    // without the necessary page resources and then re-render once those come in.
    emitter.on(`onPostLoadPageResources`, e => {
-      if (e.page.path === loader.getPage(this.state.location.pathname).path) {
+      if (
+        loader.getPage(this.state.location.pathname) &&


This could possibly be that the page resource might not exist. Putting a checking to guard from having to encounter error at this part.

efallancy · 2017-10-31T17:22:22Z

Hi @KyleAMathews 😃

Based on this line of code for registering service worker, is there a way that we sort of being able to not always give a false alarm preventing the window to be reloaded? It seems to trigger multiple re-rendering in ComponentRenderer or as long as page not found (called bygetResourcesForPathname through findPage method), that line of code will cause the window to reload, though we do know that the page does not exist.

If we would like to preserve using window.___history or window.___navigateTo, what I can see is that we need to call the callback function to be able to ensure the history is attached to the window. Other than that, the alternative is to use window.location.href (which I know is not favourable) 😅

Let me know what you have in mind as I could have overlook something 😺

Edit: Out of curiosity, I notice we have this line here to check for resources and run the DOM rendering through callback. Any reason for that being implemented that way compared to implementing it not through callback? Just like how it's done in root file 😅

…fallancy/gatsby into topic/fix-404-navigate-page

…not found

… serving 404 content is not feasible and would return unexpected result

… ComponentRenderer

KyleAMathews · 2017-11-02T16:00:58Z

Hey, what's the status of this?

gatsbybot · 2017-11-02T16:12:12Z

Deploy preview ready!

Built with commit a4feba9

https://deploy-preview-2667--using-drupal.netlify.com

efallancy · 2017-11-02T23:10:19Z

Hi @KyleAMathews , I am not particularly sure how to go forth from here. I would need your advice on this. Here's what I found:

In order to be able to ensure history is attached to window, we will need to execute the callback inside loader.js in line 251 . However, this would cause the page to reload and causes the flash in the screen, just like how it's being describe in Fix 404 navigation #2000 . This is happening due to unregistering of service worker inside getResourcesForPathname. getResourcesForPathname is being called inside component-renderer and production-app.
To attach the history in top level might not work if no DOM being mounted and the 404 page that being served are just static page.
The last resort would be on using window.location.href if window.___history not defined. This might not be preferable though 😐

Also referring to the old comment above, I notice that the DOM rendering has different approach compared to production and non production environment.

I hope you'll be able to point out what I went wrong or what did I miss from here.

efallancy · 2017-11-04T13:40:36Z

@KyleAMathews I've tried to prevent multiple refresh when page not found. The glitch that appears on the page when page not found is caused by window.location.reload whenever the code passes through the unregistering of service worker (found in getResourcesForPathname in loader.js) . I made a check to only reload the page whenever there's a service worker to be unregister. As for attaching of history, I suppose it should work fine on your end too. It pretty much uses the same way as how it's initially proposed in #2000

Let me know what you think or have in mind 😃

Edit: Apologies 🙇 I might get it wrong as I notice reloading also triggered by the emitter onPostLoadPageResources. Also notice multiple remounting behaviour too.

… route

revolunet · 2017-11-05T00:15:34Z

thanks for working on that 👍 😍

KyleAMathews · 2017-11-06T18:03:31Z

packages/gatsby/cache-dir/loader.js

@@ -249,7 +254,8 @@ const queue = {

      if (!page) {
        console.log(`A page wasn't found for "${path}"`)
-        return
+        cb()


Shouldn't this just be return cb()?

@KyleAMathews ah yes. You're right. I need to update this to be return cb()

KyleAMathews · 2017-11-06T18:05:27Z

Well looks like you're making progress :-)

Can go to https://deploy-preview-2667--gatsbyjs.netlify.com/sdfsdf now and click a link.

Not really sure what the blinking is from… looked a bit but don't have time for a deeper dive.

I'd be fine merging this if you're running out of time to keep investigating as it's an improvement over what we have now or feel free to keep searching.

efallancy · 2017-11-07T10:17:53Z

@KyleAMathews I'm happy for you to merge it if you're happy with it 😄

I can do the search outside on different issue or PR. Also, if the inability to click on any link when page not found in production has impact on most app, I reckon we might need to merge.

KyleAMathews · 2017-11-13T20:09:11Z

Thanks for all your work @emmafallancy! Let's put this out there!

revolunet · 2017-11-13T21:25:49Z

thanks ! 👏

efallancy · 2017-11-13T23:27:35Z

Thanks @KyleAMathews 😄

ferdinandsalis · 2017-11-16T22:59:28Z

@emmafallancy just a heads up I am on v1.9.114 and I am getting this error on a client-only route,
Uncaught TypeError: Cannot read property 'push' of undefined. production-app.js:87:6. The client route is rendered — initially showing the not found page. It seems this issue persists at least with client only routes. Any ideas what might be wrong here?

efallancy · 2017-11-18T15:14:32Z

Hi @ferdinandsalis , thanks for the heads up 😄
Based on the error that you've given, it seems like it couldn't find history that supposed to be attached during initial DOM rendering. It should be made available if you're navigating from a page that has already been rendered.

Would you be able to confirm if this is still happening in v1.9.117 ?

ferdinandsalis · 2017-11-18T22:31:07Z

Its still happening with v1.9.117.

efallancy · 2017-11-19T00:37:28Z

Do you have a sample repo that I can have a look at or poke around? I tried poking around with Gatsby's website and it seems that I can navigate around without hitting into the error that you mentioned. I can do a PR but I would actually like to understand how you managed to hit into that error 😅

@KyleAMathews have you encounter this issue before?

What I can confirm is that if the 404 page uses layout, and when user navigate to other page from 404 page and going back again (by pressing back button), the layout will be gone and only renders the content of the 404 page. I can put a PR to this, along with the issue that you've found out.

ferdinandsalis · 2017-11-19T08:36:54Z

@emmafallancy thanks for taking a look at this. I will try to get repo up that reproduces the issue. Thanks for your help.

Fix url navigation when window.___history does not exist

e411564

efallancy commented Oct 29, 2017

View reviewed changes

Merge branch 'master' into topic/fix-404-navigate-page

5fa690e

efallancy added 3 commits October 31, 2017 13:09

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

3e11d32

…c/fix-404-navigate-page

Merge branch 'topic/fix-404-navigate-page' of https://github.com/emma…

ea97aa2

…fallancy/gatsby into topic/fix-404-navigate-page

Setting window.___history in top level to make it accessible even dur…

d6ee768

…ing page not found

efallancy added 3 commits November 1, 2017 03:39

Add canges to ensure we default to send 404 page resources when no pa…

b6a6bba

…ge found during production and ensure a safe guard in component renderer checking

Reverting to instead calling a callback

d6e62a8

Merge branch 'master' into topic/fix-404-navigate-page

443defc

efallancy commented Oct 31, 2017

View reviewed changes

efallancy added 2 commits November 1, 2017 05:03

Returning 404 content instead of calling callback

49aa71a

Merge branch 'topic/fix-404-navigate-page' of https://github.com/emma…

9b90549

…fallancy/gatsby into topic/fix-404-navigate-page

alxshelepenok mentioned this pull request Oct 31, 2017

Link on 404 page is not working alxshelepenok/gatsby-starter-lumen#46

Closed

efallancy and others added 6 commits November 1, 2017 20:25

Replace to use let instead of const for reassigning variable if page …

09f189e

…not found

Revert to use cb to ensure history attached in production as directly…

3b87875

… serving 404 content is not feasible and would return unexpected result

Revert from using let to const in loader file

894f651

update location pathname for 404 on displaying 404 page in router for…

729453d

… ComponentRenderer

Replace to use 404.html instead

68b166d

Merge branch 'master' into topic/fix-404-navigate-page

086db8b

efallancy added 4 commits November 4, 2017 22:50

Remove lookup of resources and directly render the DOM

0441732

Add return value for component renderer

efd14d1

Add checking to prevent multiple reloading of the window

8121d38

Revert changes to instead use callback

b621276

efallancy added 2 commits November 5, 2017 03:47

Temporarily removing resource fetching since handle through rendering…

ad8dc08

… route

Revert changes

5285257

efallancy added 3 commits November 5, 2017 21:06

Merge branch 'master' into topic/fix-404-navigate-page

8526fa3

Replace default layout component to take in default layout

1147732

Update to accept default layout for page not found during production

f84acb6

KyleAMathews reviewed Nov 6, 2017

View reviewed changes

efallancy added 2 commits November 7, 2017 21:18

Merge branch 'master' into topic/fix-404-navigate-page

54281ae

Update loader file

a4feba9

revolunet mentioned this pull request Nov 8, 2017

Navigating from 404 page errors on production deployment #1838

Closed

KyleAMathews merged commit faffba9 into gatsbyjs:master Nov 13, 2017

efallancy deleted the topic/fix-404-navigate-page branch November 28, 2017 23:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix url navigation when window.___history does not exist #2667

Fix url navigation when window.___history does not exist #2667

efallancy commented Oct 29, 2017 •

edited

Loading

gatsbybot commented Oct 29, 2017 •

edited

Loading

efallancy Oct 29, 2017

KyleAMathews commented Oct 30, 2017

KyleAMathews commented Oct 30, 2017

efallancy commented Oct 31, 2017

efallancy commented Oct 31, 2017 •

edited

Loading

KyleAMathews commented Oct 31, 2017

efallancy commented Oct 31, 2017

efallancy Oct 31, 2017

efallancy Oct 31, 2017

efallancy commented Oct 31, 2017 •

edited

Loading

KyleAMathews commented Nov 2, 2017

gatsbybot commented Nov 2, 2017 •

edited

Loading

efallancy commented Nov 2, 2017

efallancy commented Nov 4, 2017 •

edited

Loading

revolunet commented Nov 5, 2017

KyleAMathews Nov 6, 2017

efallancy Nov 7, 2017

KyleAMathews commented Nov 6, 2017

efallancy commented Nov 7, 2017

KyleAMathews commented Nov 13, 2017

revolunet commented Nov 13, 2017 •

edited

Loading

efallancy commented Nov 13, 2017

ferdinandsalis commented Nov 16, 2017

efallancy commented Nov 18, 2017

ferdinandsalis commented Nov 18, 2017

efallancy commented Nov 19, 2017

ferdinandsalis commented Nov 19, 2017

Fix url navigation when window.___history does not exist #2667

Fix url navigation when window.___history does not exist #2667

Conversation

efallancy commented Oct 29, 2017 • edited Loading

gatsbybot commented Oct 29, 2017 • edited Loading

efallancy Oct 29, 2017

Choose a reason for hiding this comment

KyleAMathews commented Oct 30, 2017

KyleAMathews commented Oct 30, 2017

efallancy commented Oct 31, 2017

efallancy commented Oct 31, 2017 • edited Loading

KyleAMathews commented Oct 31, 2017

efallancy commented Oct 31, 2017

efallancy Oct 31, 2017

Choose a reason for hiding this comment

efallancy Oct 31, 2017

Choose a reason for hiding this comment

efallancy commented Oct 31, 2017 • edited Loading

KyleAMathews commented Nov 2, 2017

gatsbybot commented Nov 2, 2017 • edited Loading

efallancy commented Nov 2, 2017

efallancy commented Nov 4, 2017 • edited Loading

revolunet commented Nov 5, 2017

KyleAMathews Nov 6, 2017

Choose a reason for hiding this comment

efallancy Nov 7, 2017

Choose a reason for hiding this comment

KyleAMathews commented Nov 6, 2017

efallancy commented Nov 7, 2017

KyleAMathews commented Nov 13, 2017

revolunet commented Nov 13, 2017 • edited Loading

efallancy commented Nov 13, 2017

ferdinandsalis commented Nov 16, 2017

efallancy commented Nov 18, 2017

ferdinandsalis commented Nov 18, 2017

efallancy commented Nov 19, 2017

ferdinandsalis commented Nov 19, 2017

efallancy commented Oct 29, 2017 •

edited

Loading

gatsbybot commented Oct 29, 2017 •

edited

Loading

efallancy commented Oct 31, 2017 •

edited

Loading

efallancy commented Oct 31, 2017 •

edited

Loading

gatsbybot commented Nov 2, 2017 •

edited

Loading

efallancy commented Nov 4, 2017 •

edited

Loading

revolunet commented Nov 13, 2017 •

edited

Loading