Restart on lock file changes

[matthewp]

Changes

• Adds common package lock files to the list of files to watch for trigger a dev server restart.

Testing

• New unit test case added

Docs

N/A

[changeset-bot]

🦋 Changeset detected

Latest commit: 48d3e4e

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

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]

This PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.

doesn't contain a minor

[bluwy]

I think there was a conversation about this during the offsite, but I don't quite recall 😅 What is the reason for this again? Restarting the dev server if the lockfile changes doesn't always bring the desired behaviour. For example:

1. Project imports foo library
2. Astro dev server starts which imports foo library
3. Node.js caches its import
4. User updates foo library while Astro dev server is active
5. Astro dev server restarts
6. foo library is still outdated because Node.js caches its import

It'll work for client-only libraries though I believe, but I don't think the same would happen for node/ssr. The better way in this case is to completely manually restart the dev server.

[Princesseuh]

If we can't make it work, maybe we could at least output a message like "Lockfile changes detected, restart your dev server to see changes" or something similar?

It'd be cool if it actually worked, though.

[bluwy]

It could sometimes work though, like adding a new dependency that wasn't cached before, and after a server restart you can start using the dependency. I think Matthew fixed it in Vite to allow this flow to happen. But it's hard to tell usually which case is happening and whether it's safe.

I guess there's the point where we already watch the package.json file (which has the same problems), so it doesn't hurt to watch more lock files, but I feel like we should encourage users to manually restart the server instead of potentially leading them to an odd server state.

[ematipico]

I agree with @bluwy here. I am not sure what we're fixing because the description of the PR doesn't say anything about it. Regardless, I would encourage users to restart the server to avoid possible side-effects

[ematipico]

package.json isn't a lock file, is it a mistake?

[matthewp]

Good points, I think what we should actually be doing here is restarting the process, not just restarting the container, because of the cached import issue. To do this I think we'll need to make it so that there's a parent process that can handle the restart.

[bluwy]

As a stop gap now, I'm fine with merging this to help with some cases. I was thinking it's worth mentioning that this doesn't fix all the issues with adding/updating packages.

[Fryuni]

To do this I think we'll need to make it so that there's a parent process that can handle the restart.

I think that should be easily doable by forking on the installable astro.js before importing the CLI index. We can then define an exit code ro represent a request for a restart as even use Node's built-in IPC

[bluwy]

Refactoring to parent-child process to handle complete restarts feel a bit over board to me and potentially a can of worms. I think it's fine to keep the current flow as you're not editing the package.json or lock files all the time.