Fix failed server restart calling integration hooks twice

[bluwy]

Changes

Fix #7827

When restarting the server and config load fail, don't re-create the server. Re-use the existing (already running) server instead.

Testing

Tested manually. Tried adding a test, but it seems like config loading is borked. It shouldn't have based it's root on fixtures/alias because it always reads that fixture's astro config.

Docs

n/a. bug fix.

[changeset-bot]

🦋 Changeset detected

Latest commit: c0e35b1

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

[matthewp]

Why move this out of the package? I think this function is sort of implementation detaily

[bluwy]

It's only used by tests and I'm not really a fan of putting tests related logic in the source code 😬

[matthewp]

If this were a class with a isStarted method would you feel that way too?

[bluwy]

Yeah, right now the Container concept has things like handle(), and Restart (container) has the restarted property, which are only used in tests, and it feels like we're prying out internal implementation for restarting so we can unit test it.

I also realized the unit tests wasn't testing things right (e.g. invalid config error), plus uses hacky methods like:

astro/packages/astro/test/units/dev/restart.test.js

Lines 68 to 72 in 6806629

	// Vite watches the real filesystem, so we have to mock this part. It's not so bad.
	restart.container.viteServer.watcher.emit(
	'change',
	fs.getFullyResolvedPath('/astro.config.mjs')
	);

which is not so great. I think server restarts as a feature is a user experience, and it should've been tested through e2e in the first place. Sorry for going into a rant 😅 I've been slowing trying to fix these e.g. in the recent config refactors.

[matthewp]

Yeah, right now the Container concept has things like handle(), and Restart (container) has the restarted property, which are only used in tests, and it feels like we're prying out internal implementation for restarting so we can unit test it.

Yes, we are! Testing is very important here, we had a lot of HMR bugs prior to this refactor and now we see very few. So pretty or not, it was a success.

And although these things are not currently exposed as APIs doesn't mean they won't or shouldn't be. A server being able to restart itself seems like a pretty essential feature. Real production servers like Apache and Nginx have these APIs.

I think server restarts as a feature is a user experience, and it should've been tested through e2e in the first place. Sorry for going into a rant 😅 I've been slowing trying to fix these e.g. in the recent config refactors.

e2e tests would be much slower though. As developers on libraries/frameworks we have to take on ugly/not great code so that our users can write clean code themselves. Always remember that for us the order of priority is:

1. Well tested code
2. Fast tests (which allows you to do the first)
3. Clean code / abstractions

We have to do some ugly things because the libraries we depend on (Vite in this example) are themselves not designed to be testable. We can't let our libraries dictate that we not be as well.

---

But, this is a bit of a sidebar, happy to keep talking about it elsewhere 😀. Would love to have this API improved and be less ugly as long as we don't sacrifice test exhaustion or speed in order to do so.

For this particular change I don't see a problem with removing the isStarted function since we're not longer using it and, as you pointed out, we already are reaching into some internals for the tests anyways.