Skip to content
This repository has been archived by the owner on May 10, 2021. It is now read-only.

Unknown routes should render 404.html #2

Closed
gamliela opened this issue Mar 16, 2020 · 11 comments
Closed

Unknown routes should render 404.html #2

gamliela opened this issue Mar 16, 2020 · 11 comments

Comments

@gamliela
Copy link

Hi,

Cool project! I was looking for something similar for quite long, and your solution seems to work pretty well!

The current implementation doesn't support rendering of the 404.html file, if an unknown URL is requested. For example, try this link: https://next-on.netlify.com/some-non-existing-url - it should actually render _next/pages/404.html file.

A quick fix that I've found for this problem is to simply copy the 404.html file to public in a post-build step. Something like cp public/_next/pages/404.html public. The reason is that Netlify are expecting to find this file exactly on this place. But I wonder if that's the right solution, or if there are better ways. Another approach that I've tried was to add /* /_next/pages/404.html 404 at the end of _redirects file, but it didn't work very well with netlify-dev.

Would love to hear your thoughts. Thanks :)

FinnWoelm added a commit that referenced this issue Apr 19, 2020
- Add support for catch-all routes (#1, #2)
- README: Fix instructions for local preview
FinnWoelm added a commit that referenced this issue Apr 19, 2020
- Add support for catch-all routes (#1, #2)
- README: Fix instructions for local preview
@FinnWoelm
Copy link
Collaborator

Hi @gamliela, thank you for opening this issue and for sharing solutions of your own!

I will look into it this week and get back to you soon. I'd be quite interested to hear how you ended up dealing with it after all: By copying the 404.html to public after building or by adding the redirect?

PS: Apologies for taking so long to get back. Somehow it totally slipped my attention.
PPS: I accidentally tagged this issue in the latest release, when I meant to tag the second pull request (#5) instead. 😶

@gamliela
Copy link
Author

No worries :) Yes, I simply copy the 404.html file. Redirect didn't work well for me. Just thought it could be nice to add that feature, since it's already moving a lot of files around.

@FinnWoelm
Copy link
Collaborator

Hmmm, very true. I like the idea!

Two considerations:

  • If there is already a user-created 404.html file in the public folder, we don't want to overwrite that. We skip copying the 404.html in that case and print out a warning?

  • If the NextJS 404 page is dynamic (uses getInitialProps), we need to route the request to a Netlify function. So in this case, we need to add that redirect you mentioned. This will break locally previewing with netlify dev, so we need to figure something out... Any ideas? I wish netlify dev was consistent with Netlify in how it handles redirects.

@gamliela
Copy link
Author

Good points!

If there is already a user-created 404.html file in the public folder, we don't want to overwrite that. We skip copying the 404.html in that case and print out a warning?

Probably not overriding - what is the existing behaviour (for example when generating the _redirects file?)

If the NextJS 404 page is dynamic (uses getInitialProps), we need to route the request to a Netlify function. So in this case, we need to add that redirect you mentioned. This will break locally previewing with netlify dev, so we need to figure something out... Any ideas? I wish netlify dev was consistent with Netlify in how it handles redirects.

I am not sure that 404 page can be dynamic, it is a safe fallback mechanism that is supplied by Nelify (not next.js) when no other route could be found. As Netlify not aware to dynamic content, I'm not sure you can make this page dynamic. I tried catch-all rule at the end of the routes file, but I don't remember now if it ever worked for me...

@FinnWoelm
Copy link
Collaborator

Hey @gamliela,

This is finally being fixed in the upcoming next-on-netlify v2.

I've already written the code and tests; it works wonderfully. There are a few more things I want to look into before releasing v2, but I'm getting very close!

Finn 🙂

PS:

I am not sure that 404 page can be dynamic, it is a safe fallback mechanism that is supplied by Nelify (not next.js) when no other route could be found. As Netlify not aware to dynamic content, I'm not sure you can make this page dynamic. I tried catch-all rule at the end of the routes file, but I don't remember now if it ever worked for me...

You were totally right on this. I was confusing it with the pages/_error.js page.

@gamliela
Copy link
Author

Thanks for the update! great news :)

I've tested wip-version2 branch. I guess it's still work on progress, but if you want I can share some of my findings.

  1. I got error Error: 'out/index.html' already exists when I use /pages/index.jsx file. When I rename the file to /pages/index2.jsx it works;
  2. I got 404.html generated on the project root, but also on /out directory. Is this by intent? I would suggest to avoid generate files on the project root as default behaviour;
  3. Maybe worth to update documentation about what should be added to .gitignore. Not sure about that, but at least in my V1 configuration I had this block:
# auto-generated files by build/next-on-netlify
/functions/nextRouter
/public/_next
/public/404.html
/public/_redirects

My test project is at https://github.com/gamliela/playground-auth/tree/next-on-netlify-v2

Thanks for the awesome work! looking forward seeing the final version :)

FinnWoelm added a commit that referenced this issue May 31, 2020
When we copy the pre-rendered pages to `out/`, we do a check to make
sure that the file does not already exist. This is to make sure that we
don't silently overwrite one of the files copied from `public/`, for
example.

But for `index.js` files, Netlify always creates two page entries, one
routing to `/` and one routing to `/index`. So in this case, the first
copying of `index.js` to `out/` goes fine. The second copying fails
because `index.js` already exists.

We fix this by first filtering our list of pages for unique pages and
then performing the copy operation.

Fixes: #2 (comment)
@FinnWoelm
Copy link
Collaborator

Hey @gamliela,

Wow, thanks for checking it out so quickly and looking for issues!

  1. OH!! You are right. When I copy the pre-rendered pages to out/, I do a check to make sure that the file does not already exist. This is to make sure that I don't silently overwrite one of the files copied from public/.

    But for index.js files, Netlify always creates two page entries, one routing to / and one routing to /index. So in this case, the first copying of index.html to out/ goes fine. The second copying fails because index.js already exists.

    I am so glad you caught that. In my tests, the index.js files were always using getInitialProps, so the issue didn't show up 😶 I just fixed it and added a test to make sure it doesn't happen again in the future! If you get another chance to have a look, let me know if it works now.

  2. The duplicate 404.html page is on purpose. There is an inconsistency between deployed Netlify and netlify-cli dev. Currenly, Netlify (deployed) expects the 404.html file in the publish folder, while netlify-cli dev expects it at the directory root.

    In order to cover both cases, I'm copying the 404.html to both locations until this is fixed. I want to file an issue with https://github.com/netlify/cli, but haven't had a chance yet. Feel free to go for it, if you want 😀

  3. Yes, things changed a bit! The wip-version2 README should be up to date: https://github.com/FinnWoelm/next-on-netlify/blob/wip-version2/README.md

    Notable changes:

    • The publish folder in netlify.toml should now be out/
      next-on-netlify now copies pre-rendered pages and static assets and the contents of the public/ directory into out/. This way, we don't have to worry about messing up the public/ folders and it is consistent with what next export does.
    • For local previewing, you should use netlify-cli v2.51.0 or later. Lots of inconsistencies between deployed Netlify and netlify dev have been fixed!
    • You no longer need a 3rd party library such as serve or http-server for local previewing. netlify-cli comes with its own. But you do need to change the netlify.toml configuration:
      [dev]
        functions = "functions"
        publish   = "out"
        # We manually set the framework to static, otherwise Netlify automatically
        # detects NextJS and redirects do not work.
        # Read more: https://github.com/netlify/cli/blob/master/docs/netlify-dev.md#project-detection
        framework = "#static"
    • If you do local previewing, you .gitignore should now cover:
      # Files generated by next-on-netlify command
      functions/nextRouter
      out/
      404.html

Again, thanks for being so quick to take a look! 🙌 You definitely caught a major issue there! Keep up the search for more 😁

I am going to look into getStaticProps and getServerSideProps now and see if there is something we can do.

@gamliela
Copy link
Author

  1. Great!
  2. Sounds like an issue, not sure about this workaround, let's hope it will be resolved soon.
  3. I wasn't aware to all these changes. It seems that I need to read the README again :)

I'll continue playing with it and let you know if I find anything else. Thanks!

FinnWoelm added a commit that referenced this issue Jun 2, 2020
- **Breaking: You must change your `netlify.toml` configuration for
  next-on-netlify v2.0.0.** Please look at the README for the latest
  configuration.
  `next-on-netlify` now builds pre-rendered pages and static assets in
  `out_publish`. Netlify Functions for SSR pages are built to
  `out_functions`.
- Add support for `getStaticProps` (#7)
- Add support for `getStaticPaths` with and without fallback (#7)
- Add support for `getServerSideProps` (#7)
- Query string parameters are now correctly passed to Next Pages and API
  endpoints (#9)
- Response headers are now correctly set (#9)
- When a user encounters a 404, `next-on-netlify` now display the NextJS
  404 page rather than Netlify's default 404 page. You can customize the
  NextJS 404 page. (#2)
- Every page with server-side rendering is now converted to a
  stand-alone Netlify Function. Previously, all SSR pages were bundled
  in a single Netlify Function.
- `next-on-netlify` now prints out which pages are being converted to
  Netlify Functions for SSR, which pages are served as pre-rendered
  HTML, and the redirects that are being generated.
- Adding custom redirects via a `_redirects` file in the project root is
  no longer supported. Let me know if you want this back. Or define your
  redirects in `netlify.toml`.
@FinnWoelm
Copy link
Collaborator

Hey @gamliela,

next-on-netlify v2 is finally out 🎉 🎉 You can see the full list of changes in the changelog.

Thank you for your bug-hunting and for brainstorming the best way to tackle the 404-page!

Unfortunately, I had to change the configuration again. So you'll need to make some changes to your netlify.toml for v2:

# netlify.toml

[build]
  command   = "npm run build"
  functions = "out_functions"
  publish   = "out_publish"

[dev]
  functions = "out_functions"
  publish   = "out_publish"
  framework = "#static"

And you will want to adjust your .gitignore for local preview:

# .gitignore

# Files generated by next-on-netlify command
/out_publish/
/out_functions/
/404.html

Let me know if you run into any trouble. I closed this issue for now, but let's reopen it at any time if necessary! I've made a note to file an issue with netlify-cli about the 404.html being at directory root vs being in the publish folder and will keep you posted!

If you build something awesome, let me know! I'd love to feature some real-world projects in the README.

Happy hacking! 🔥
Finn

@FinnWoelm
Copy link
Collaborator

I know it's been a while, but I wanted to share a quick update:

I finally wrote a fix for netlify/cli that picks up the 404.html from the /out_publish folder (matching the behavior of netlify.com). This was merged into netlify/cli a few days ago. The latest next-on-netlify (v2.4.0) no longer generates a 404.html at the project root! 😊

@gamliela
Copy link
Author

gamliela commented Sep 6, 2020

Unbelievable! great news, thank you very much :)

3H925 added a commit to 3H925/next-on-netlify that referenced this issue Mar 15, 2024
For Netlify (deployed), the 404.html page in the out/ folder already 
works. For netlify dev, the 404.html page must be at the directory root.

Fixes: netlify/next-on-netlify#2
3H925 added a commit to 3H925/next-on-netlify that referenced this issue Mar 15, 2024
When we copy the pre-rendered pages to `out/`, we do a check to make
sure that the file does not already exist. This is to make sure that we
don't silently overwrite one of the files copied from `public/`, for
example.

But for `index.js` files, Netlify always creates two page entries, one
routing to `/` and one routing to `/index`. So in this case, the first
copying of `index.js` to `out/` goes fine. The second copying fails
because `index.js` already exists.

We fix this by first filtering our list of pages for unique pages and
then performing the copy operation.

Fixes: netlify/next-on-netlify#2 (comment)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants