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

[v2] [cache-dir] Use 404 resources in EnsureResources if the page doesn't exist #8510

Merged
merged 6 commits into from
Sep 26, 2018
Merged

[v2] [cache-dir] Use 404 resources in EnsureResources if the page doesn't exist #8510

merged 6 commits into from
Sep 26, 2018

Conversation

vtenfys
Copy link
Contributor

@vtenfys vtenfys commented Sep 24, 2018

  • Get the 404 resources in ensure-resources.js
  • Separate code in load-directly-or-404.js to prevent code reuse
  • Return 404 resources from getResourcesFromPathnameSync if the page isn't found, to match behaviour of the async version
  • Minor refactors to improve performance based on the changes made

Fixes #8297

@vtenfys vtenfys changed the title [v2] [cache-dir] Use 404 resources in EnsureResources if the page doesn't exist [v2] [cache-dir] [WIP] Use 404 resources in EnsureResources if the page doesn't exist Sep 24, 2018
Copy link
Contributor

@KyleAMathews KyleAMathews left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

window.location.replace(url)

const url = getRedirectUrl(this.state.location.pathname)
if (url) window.location.replace(url)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the non-shorthand form?

@@ -16,6 +17,7 @@ class EnsureResources extends React.Component {
this.state = {
location: { ...location },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where did we ever end up on this? Is this needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah, probably not! just don't use git blame - it tells lies!

@pieh
Copy link
Contributor

pieh commented Sep 24, 2018

I chatted with @davidbailey00 a little and we (or just him) will iterate on this tomorrow

@vtenfys vtenfys requested a review from a team as a code owner September 25, 2018 14:57
@vtenfys vtenfys changed the title [v2] [cache-dir] [WIP] Use 404 resources in EnsureResources if the page doesn't exist [v2] [cache-dir] Use 404 resources in EnsureResources if the page doesn't exist Sep 26, 2018
Copy link
Contributor

@pieh pieh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@pieh pieh merged commit c42924a into gatsbyjs:master Sep 26, 2018
lipis added a commit to lipis/gatsby that referenced this pull request Sep 26, 2018
* 'master' of github.com:gatsbyjs/gatsby:
  feat(cache-dir): Add a button for custom 404s in development (gatsbyjs#8548)
  fix: add ecosystem team for starters YAML
  fix: fix displaying and hydrating 404 page in production builds (gatsbyjs#8510)
  fix: don't expect `application/json` type nodes to be files (gatsbyjs#8544)
  Delete cds-takeaways.md (gatsbyjs#8552)
  chore(release): Publish
  chore: remove npm-run-all dep and update deps in e2e tests (gatsbyjs#8529)
  [www] Site/Starter Showcase refactor, rinse II (gatsbyjs#8411)
  Rfc process doc (gatsbyjs#8537)
  [www] a11y improvements (gatsbyjs#8536)
  Update the GraphQL stitching blog post & docs (gatsbyjs#8516)
  fix: change teams to reduce noise for people
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants