fix(cli): #1766 return a 404 when a develop page file is deleted while the server runs - #1773
Open
jstockdi wants to merge 3 commits into
Open
Conversation
… is deleted while the server runs
… so build failures stay loud
…ping the develop test server
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related Issue
Resolves #1766
Documentation
No documentation changes. This corrects an HTTP status code in
greenwood develop; the documented behaviour is unchanged.Summary of Changes
Deleting a page file while the dev server is running returned a 500 with an empty body, plus a raw
ENOENTstack in the terminal, where a 404 is correct.The page graph is generated once at startup, so a deleted page's node is still present.
shouldServein the standard HTML resource plugin still matches the route,servethen reaches an unguardedfs.readFile, and the resultingENOENTis swallowed by the catch-all inlifecycles/serve.js, which sets a 500. Restartingdevelopmakes the same route correctly 404.The fix guards that one read: in
develop, anENOENTreturns a404response; any other error still throws so real failures are not masked.The
developgate is load-bearing rather than cosmetic.lifecycles/prerender.jscallsplugin.serve()on every resource plugin and does not checkresponse.ok, so an ungated 404 here is swallowed duringgreenwood build: a page deleted between graph generation and prerender produced exit0, agenerated page...log line, and an emptypublic/<route>/index.html. Greenwood's convention for a missing page file at build time is a hard failure (lifecycles/graph.js:179), and the stale-graph condition that justifies a 404 only exists indevelop, so the guard is scoped to match.Scope
Deliberately narrow. This does not refresh the graph or add a file watcher — hot reloading of the graph (which is also what makes a newly added page 404 until restart) is tracked in #1278 and is a feature. This is just the incorrect status code and unhandled read error on delete, which is wrong regardless of whether the graph ever refreshes.
It also does not change
greenwood buildorgreenwood serve.buildstill fails loudly on a missing page file, verified by test;servenever reaches this plugin at all, sincelifecycles/config.js:36-38excludesplugin-standard-htmlfromisStandardStaticResource.Testing
Folded into the existing
develop.defaultcase rather than a new directory. Afixtures/deleted.htmlis copied tosrc/pages/deleted.htmlvia gallinago'ssetupFilesbefore the server starts (so it is in the graph), the test asserts it serves200, deletes it while the server is still running, and asserts the same route now returns404. Teardown removes the copy if a failure leaves it behind.There is no timing element —
fs.rmSynccompletes before the secondfetch— so the result is deterministic. Each of the counts below is from 5 consecutive runs ofpackages/cli/test/cases/develop.default:AssertionError: expected 500 to equal 404Baseline before any change was 115 passing; the new describe adds 3 tests.
Wider runs, all green with the fix:
develop.*cases: 313 passingbuild.default,build.default.pages*,serve.default: 48 passingprettier --checkandeslintare clean on the changed files, as isls-lint.Note for the maintainer
The fix for #1770 lands in this same file,
packages/cli/src/plugins/resource/plugin-standard-html.js, but at thegetMatchingDynamicSsrRoute(...) || {}expression around line 42 — well clear of this hunk around line 56, so the two should merge without conflict.