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

Allow prerendering NotFoundPage as 404.html #1891

Merged
merged 8 commits into from
Mar 8, 2021

Conversation

dac09
Copy link
Collaborator

@dac09 dac09 commented Mar 1, 2021

What?

  • Allows generation of 404 pages from NotFoundPage in Routes.js/tsx
  • Also changes defaultIndex.html -> 200.html to follow same convention

Why 404.html?
https://docs.netlify.com/routing/redirects/redirect-options/#custom-404-page-handling

@dac09 dac09 marked this pull request as draft March 1, 2021 17:16
@dac09 dac09 added this to the next release milestone Mar 1, 2021
@thedavidprice
Copy link
Contributor

Curious if we should add this as a default setup for new projects (i.e. prerender NotFoundPage) — is there a downside?

@dac09
Copy link
Collaborator Author

dac09 commented Mar 1, 2021

Curious if we should add this as a default setup for new projects (i.e. prerender NotFoundPage) — is there a downside?

Only downside is they're not prepared for their NotFoundPage to be prerendered. Maybe they use a library that doesn't work in Node.js?

@dac09 dac09 marked this pull request as ready for review March 1, 2021 19:30
mockedRoutes = [
{
name: 'bazinga',
path: '/bazinga',
name: undefined,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is how structure outputs NotFoundPages

Copy link
Contributor

Choose a reason for hiding this comment

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

would path: "some-unknown-path" not be more clearly indicate intent? Or doesn't that work in this case?

Copy link
Collaborator Author

@dac09 dac09 Mar 5, 2021

Choose a reason for hiding this comment

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

Hey 👋 Joeren!

We're mocking the output of the redwoodjs/structure package here. This is just what it outputs, when it comes across a route with a not found prop.

      <Route notfound page={NotFoundPage} prerender />

The code that deals with that actually doesn't really care about the values of name/path, i've just made this change to more accurately represent what structure does.

@thedavidprice
Copy link
Contributor

Only downside is they're not prepared for their NotFoundPage to be prerendered. Maybe they use a library that doesn't work in Node.js?

Understood that could be unexpected and hard to diagnose, especially given that at this time there would be no errors locally but only at the time of deploy. Even though it wouldn't be breaking, definitely not ideal.

This gives me a couple of ideas I'll table for now. Thanks and carry on!

@github-actions
Copy link

github-actions bot commented Mar 4, 2021

📦 PR Packages

Click to Show Package Download Links

https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/create-redwood-app-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-api-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-api-server-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-auth-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-cli-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-core-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-dev-server-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-eslint-config-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-eslint-plugin-redwood-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-forms-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-internal-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-prerender-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-router-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-structure-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-testing-0.26.2-dbc9d6e.tgz
https://rw-pr-redwoodjs-com.s3.amazonaws.com/1891/redwoodjs-web-0.26.2-dbc9d6e.tgz

Install this PR by running yarn rw upgrade --pr 1891:0.26.2-dbc9d6e

@dac09 dac09 mentioned this pull request Mar 5, 2021
@@ -81,10 +81,10 @@ export const runPrerender = async ({
}

if (outputHtmlPath) {
// Copy default index.html to defaultIndex.html first
// Copy default index.html to 200.html first
Copy link
Contributor

Choose a reason for hiding this comment

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

🥇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants