-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
No 👏 more 👏 server 👏 restarts 👏 on 👏 config 👏 changes #4578
Conversation
🦋 Changeset detectedLatest commit: 67e22bd The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
optimizeDeps: { entries: [] }, | ||
clearScreen: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed the screen refreshes with a warning statement on our Vite server fallback. entries
prevents cache warnings, and clearScreen: false
preserves output!
configFlagPath && normalizePath(configFlagPath) === normalizePath(changedFile) | ||
: // Otherwise, watch for any astro.config.* file changes in project root | ||
new RegExp( | ||
`${normalizePath(resolvedRoot)}.*astro\.config\.((mjs)|(cjs)|(js)|(ts))$` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about .mts
and .cts
. j/k
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Therapist: cts
doesn't exist in the docs, it can't hurt you.
cts:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yas 👏
Actually it also looks like windows tests are failing somehow 🥲 |
3cb90e6
to
2d0f4ce
Compare
153acca
to
1ef563a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Ben, this is awesome!
I added some better handling for invalid configs and cleaned up the messages a little bit! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just circling back to approve this again! Should be good to merge as soon as we're ready for the next minor release.
Update: both of these are addressed! @natemoo-re actually needs a couple more things:
Both top priority tomorrow morning for me 👍 |
This reverts commit 8ca8662.
7610862
to
67e22bd
Compare
@@ -469,7 +466,7 @@ const imageUrl = 'https://www.google.com/images/branding/googlelogo/2x/googlelog | |||
``` | |||
|
|||
## Troubleshooting | |||
- If your installation doesn't seem to be working, make sure to restart the dev server. | |||
- If your installation doesn't seem to be working, try restarting the dev server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tagging @sarah11918 from my docs PR earlier. Using this verbage now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, @bholmesdev! 🥳
Changes
restartInFlight
flag to avoid clobbering when saving multiple times in a row--config
is specified, only watch changes on that file. Otherwise, watch for any newastro.config.*
files in the project root. Yes, this means your server reloads when you swap file extensions!About that
writeFile
call...All good PRs have some spice: When a config file changes, write the config contents to temporary file at the project root with a unique timestamp, and quickly delete once the config is resolved.
Proload uses a native ESM import to resolve
mjs
andjs
configs, which is great... until the file contents change. Re-importing a file in Node will not get the file's up-to-date contents; once a file is imported in an application, the contents are cached as long as the process is running. This approach writes to a new file path so Node will think it's a new import every time. This is informed by Vite's ESM loader, which does a similar write-read-then-delete strategy.I also considered writing to an internal
node_modules/.astro
directory instead of the project root. However, managing the "if this directory exists..." logic and relying onnode_modules
being present felt like unneeded overhead. This temporary file only appears briefly during development (not production), and will be removed whenever an error in thrown during config resolution (seefinally
block). Vite's comparable PR also writes to the project root for reference.Testing
TODO? Is it worth an E2E test?
Docs
TODO: check if "restart the server" recs are mentioned in the docs