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

fix(dev-server): render 404 when dynamic ssg path doesn't exist #12085

Merged
merged 9 commits into from
Apr 22, 2020

Conversation

thomasheyenbrock
Copy link
Contributor

This PR addresses the following issue: In dev mode you will be served the _error page instead of a custom 404-page if you hit a path that doesn't exist for a statically generated dynamic route. In addition, the 404-page mustn't been rendered before.

As far as I understand, in dev-mode pages are rendered "on demand", i.e. the 404-page won't be built until it is needed. The dev-server has to make sure that the page is built before attempting to render it by extending the render-methods of the NextServer and calling the ensurePage method from the hot-reloader used in dev. Then is passes on to the render-method of the NextServer.

The problem is that only the _error page was "ensured" in renderErrorToHTML, regardless of the status code and if there is a custom 404 page. I added a condition that checks if the status code is a 404 and if there is a custom 404 page. If so, we now "ensure" the 404-page, otherwise we continue using the _error page.

I also added a test suite to assert that the 404 page is indeed served in the scenario I described in the beginning. (The test for the dev-mode would fail without the changes in the dev-server.)

First time for me to work with this codebase, so I'm super excited to hear your feedback 😅 Hope the changes are fine! Also, if there's a better approach of if I missed anything I'm happy to learn 🙂

@thomasheyenbrock
Copy link
Contributor Author

thomasheyenbrock commented Apr 21, 2020

PS: I didn't open an issue here and went straight to finding a solution 😄 just let me know if I should also create a related issue 🙂

@ijjk
Copy link
Member

ijjk commented Apr 21, 2020

Stats from current PR

Default Server Mode (Decrease detected ✓)
General Overall increase ⚠️
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
buildDuration 13.2s 12.8s -431ms
nodeModulesSize 55.6 MB 55.6 MB ⚠️ +104 B
Page Load Tests Overall decrease ⚠️
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
/ failed reqs 0 0
/ total time (seconds) 2.171 2.266 ⚠️ +0.1
/ avg req/sec 1151.37 1103.21 -48.16
/error-in-render failed reqs 0 0
/error-in-render total time (seconds) 1.228 1.34 ⚠️ +0.11
/error-in-render avg req/sec 2035.27 1865.8 -169.47
Client Bundles (main, webpack, commons)
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
main-HASH.js gzip 6.25 kB 6.25 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..5e5e.js gzip 10.3 kB 10.3 kB
framework.a1..NSE.txt gzip 220 B 220 B
framework.a1..NSE.txt gzip 220 B 220 B
framework.HASH.js gzip 39 kB 39 kB
Overall change 56.7 kB 56.7 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
main-HASH.module.js gzip 4.79 kB 4.79 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.85 kB 6.85 kB
framework.HA..dule.js gzip 39 kB 39 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
polyfills-HASH.js gzip 26.2 kB 26.2 kB
Overall change 26.2 kB 26.2 kB
Client Pages
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_app.js gzip 1.23 kB 1.23 kB
_error.js gzip 3.12 kB 3.12 kB
hooks.js gzip 663 B 663 B
index.js gzip 222 B 222 B
link.js gzip 2.06 kB 2.06 kB
routerDirect.js gzip 280 B 280 B
withRouter.js gzip 278 B 278 B
Overall change 7.85 kB 7.85 kB
Client Pages Modern
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_app.module.js gzip 598 B 598 B
_error.module.js gzip 2.09 kB 2.09 kB
hooks.module.js gzip 383 B 383 B
index.module.js gzip 223 B 223 B
link.module.js gzip 1.52 kB 1.52 kB
routerDirect..dule.js gzip 279 B 279 B
withRouter.m..dule.js gzip 278 B 278 B
Overall change 5.37 kB 5.37 kB
Client Build Manifests
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Rendered Page Sizes
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
index.html gzip 929 B 929 B
link.html gzip 937 B 937 B
withRouter.html gzip 926 B 926 B
Overall change 2.79 kB 2.79 kB

Serverless Mode
General Overall increase ⚠️
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
buildDuration 13.5s 13.6s ⚠️ +55ms
nodeModulesSize 55.6 MB 55.6 MB ⚠️ +104 B
Client Bundles (main, webpack, commons)
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
main-HASH.js gzip 6.25 kB 6.25 kB
webpack-HASH.js gzip 746 B 746 B
de003c3a9d30..5e5e.js gzip 10.3 kB 10.3 kB
framework.a1..NSE.txt gzip 220 B 220 B
framework.a1..NSE.txt gzip 220 B 220 B
framework.HASH.js gzip 39 kB 39 kB
Overall change 56.7 kB 56.7 kB
Client Bundles (main, webpack, commons) Modern
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
main-HASH.module.js gzip 4.79 kB 4.79 kB
webpack-HASH..dule.js gzip 746 B 746 B
de003c3a9d30..dule.js gzip 6.85 kB 6.85 kB
framework.HA..dule.js gzip 39 kB 39 kB
Overall change 51.4 kB 51.4 kB
Legacy Client Bundles (polyfills)
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
polyfills-HASH.js gzip 26.2 kB 26.2 kB
Overall change 26.2 kB 26.2 kB
Client Pages
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_app.js gzip 1.23 kB 1.23 kB
_error.js gzip 3.12 kB 3.12 kB
hooks.js gzip 663 B 663 B
index.js gzip 222 B 222 B
link.js gzip 2.06 kB 2.06 kB
routerDirect.js gzip 280 B 280 B
withRouter.js gzip 278 B 278 B
Overall change 7.85 kB 7.85 kB
Client Pages Modern
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_app.module.js gzip 598 B 598 B
_error.module.js gzip 2.09 kB 2.09 kB
hooks.module.js gzip 383 B 383 B
index.module.js gzip 223 B 223 B
link.module.js gzip 1.52 kB 1.52 kB
routerDirect..dule.js gzip 279 B 279 B
withRouter.m..dule.js gzip 278 B 278 B
Overall change 5.37 kB 5.37 kB
Client Build Manifests
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_buildManifest.js gzip 61 B 61 B
_buildManife..dule.js gzip 61 B 61 B
Overall change 122 B 122 B
Serverless bundles
zeit/next.js canary thomasheyenbrock/next.js fix/error-instead-of-404-in-dev Change
_error.js 558 kB 558 kB
404.html 4.18 kB 4.18 kB
hooks.html 3.81 kB 3.81 kB
index.js 558 kB 558 kB
link.js 595 kB 595 kB
routerDirect.js 587 kB 587 kB
withRouter.js 587 kB 587 kB
Overall change 2.89 MB 2.89 MB

Copy link
Member

@ijjk ijjk left a comment

Choose a reason for hiding this comment

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

This change and the test cases looks good to me, thanks for the PR!

@Timer Timer added this to the 9.3.6 milestone Apr 22, 2020
@Timer Timer changed the title fix(dev-server): render 404 when ssg dynamic route doesn't exist fix(dev-server): render 404 when dynamic ssg path doesn't exist Apr 22, 2020
@Timer Timer merged commit 47d7edd into vercel:canary Apr 22, 2020
@vercel vercel locked as resolved and limited conversation to collaborators Jan 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants