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

Feat: log warning on astro.config change, restart server on astro.config added #3968

Merged
merged 3 commits into from
Jul 19, 2022

Conversation

bholmesdev
Copy link
Contributor

@bholmesdev bholmesdev commented Jul 18, 2022

Changes

  • Adds warning log to restart the server when 1. editing, 2. deleting, or 3. renaming a config file
  • Restarts the dev server when a new config file is added. This should be nice for projects using astro add to programmatically add a config!
    • Edge case: if you delete a config and add it back while the dev server is running, the dev server will not restart and a warning will not be logged. If we supported this, we could run into all sorts of issues with old config caches we haven't busted. Changing a config's file extension is correctly handled with a warning though!
astro-config-changes.mov

Testing

N/A

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jul 18, 2022

🦋 Changeset detected

Latest commit: ffd88d7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
astro Patch
@e2e/astro-component Patch
@e2e/lit-component Patch
@e2e/preact-component Patch
@e2e/react-component Patch
@e2e/solid-component Patch
@e2e/svelte-component Patch
@e2e/e2e-tailwindcss Patch
@e2e/ts-resolution Patch

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jul 18, 2022

watcher.on('add', async function restartServerOnNewConfigFile(addedFile: string) {
// if there was not a config before, attempt to resolve
if (!userConfigPath && addedFile.includes('astro.config')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I included that fuzzy addedFile.includes('astro.config') to avoid the work-intensive openConfig on most file changes.

@JuanM04
Copy link
Contributor

JuanM04 commented Jul 18, 2022

Very cool! Wouldn't it be nice if the dev server also restarts itself whenever the config is changed?

@bholmesdev
Copy link
Contributor Author

bholmesdev commented Jul 18, 2022

@JuanM04 Oh yes, that was the plan. I just hit a snag with import caching on file changes 😢 I summarized the full thread on discord, but I'll quote here:

So first, it's super easy to refresh the server itself when a config file is added, changed, or removed. The problem is actually re-importing the config. We use proload to eval the config right now, which smartly attempts to require or await import depending on your environment (see here https://github.com/natemoo-re/proload/blob/main/packages/core/lib/esm/requireOrImport.mjs)
Problem with this approach: even though the file changed, the import is fundamentally cached based on Node's importing scheme. So even trying to "reload" / re-import the config when the file contents change, the imported module won't reflect these changes 😢

The hack that many libraries use (including Vite) is slapping a ?t=${Date.now()} on the end of the import path. This can cause memory leaks overtime if you edit your config a lot, but works well in most use cases

The problem is... proload really doesn't like search params on the end of an import path. Nor should it in my opinion. So with all that said, I think I'm gonna scrap efforts to actually reload the config and log a clear message like "astro.config edited. Restart your server for changes to take effect!

☝️ I'll table this if core has any opinions! There are hacky solutions to the problem, but I'm not sure how good they feel given how close we are to RC. Just thought this problem deserved a 1 hour timebox today. The sustainable fix: add an unsafeCacheBust param to proload's load function if we really want this behavior, since some library authors might!

Copy link
Contributor

@JuanM04 JuanM04 left a comment

Choose a reason for hiding this comment

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

Nice work!

Copy link
Member

@natemoo-re natemoo-re left a comment

Choose a reason for hiding this comment

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

As they say in docsland, NWTWWHB!

@bholmesdev bholmesdev force-pushed the fix/astro-config-restart-server-log branch from fbd3c52 to ffd88d7 Compare July 19, 2022 17:18
@bholmesdev bholmesdev merged commit 95eaa20 into main Jul 19, 2022
@bholmesdev bholmesdev deleted the fix/astro-config-restart-server-log branch July 19, 2022 17:48
@astrobot-houston astrobot-houston mentioned this pull request Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants