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

Broken top navigation links at 404 page #393

Closed
jounderwood opened this issue Dec 4, 2017 · 9 comments
Closed

Broken top navigation links at 404 page #393

jounderwood opened this issue Dec 4, 2017 · 9 comments

Comments

@jounderwood
Copy link

jounderwood commented Dec 4, 2017

  1. Go to non existing page, for example - https://reactjs.org/component-api.md (I get it from react native docs "React.Component API" https://facebook.github.io/react-native/docs/state.html)
  2. Click on any top navigation link "Docs", "Tutorial", etc.

Errors in console
8929431080

Tried in latest chrome and safari.

@gaearon
Copy link
Member

gaearon commented Dec 4, 2017

Looks related to #365.
cc @KyleAMathews

@bvaughn
Copy link
Contributor

bvaughn commented Dec 4, 2017

Yeah, sounds like the same thing I'm describing in this comment. ☹️

Thanks for reporting. We'll try to dig into this soon.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 4, 2017

Unfortunately this can't be repro'd in dev mode because Gatsby handles 404s very different in dev vs prod. Shortest path to repro locally is yarn build && serve ./public.

The error is from trying to push to window.___history which doesn't exist.

My current guess is that Gatsby isn't properly executing this code which sets window.___history = history in the event of a 404. (I think the short-circuit here prevents the attachment code from running.)

@bvaughn
Copy link
Contributor

bvaughn commented Dec 4, 2017

Just as a proof-of-concept, I added a line to this short-circuit that sets a dummy window.___history:

if (!page) {
  console.log(`A page wasn't found for "${path}"`)
  window.___history = []; // TEST
  return
}

This prevents the undefined runtime error (but doesn't "fix" things since this isn't the react-router history object).

@bvaughn
Copy link
Contributor

bvaughn commented Dec 4, 2017

It's interesting to note that this defect doesn't seem to impact the Gatsby site.

Just to rule it out, I did a quick yarn upgrade-interactive bump of Gatsby and all of its plug-ins locally and...it seems to fix this issue 😅 I'll test further and submit a PR shortly.

@KyleAMathews
Copy link
Contributor

Sorry for not being able to jump on this :-( in a gazillion meetings today but I'll get on this tonight and look into why it could be undefined.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 4, 2017

No problem Kyle. I started digging into it before I realized that it wasn't happening on the Gatsby site, at which point I updated all of the Gatsby plugins and now it doesn't happen on the React site either. 😄

We could bisect to find out when it was fixed, if it's important to know.

@KyleAMathews
Copy link
Contributor

@ataylorme reported this on a site he's building where it's happening on every initial page load — with that I was able to dive into the issue and make this PR gatsbyjs/gatsby#3313

It's out in the latest gatsby release.

@bvaughn
Copy link
Contributor

bvaughn commented Dec 22, 2017

🎉 Nice!

jhonmike pushed a commit to jhonmike/reactjs.org that referenced this issue Jul 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants