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.

Sign up for GitHub

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

Fix failed server restart calling integration hooks twice #7917

Merged
merged 2 commits into from
Aug 3, 2023
+27 −30
Merged

Fix failed server restart calling integration hooks twice #7917

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7b08bee
Fix failed server restart calling integration hooks twice
bluwy Aug 2, 2023
c0e35b1
Update .changeset/metal-chairs-beam.md
bluwy Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/metal-chairs-beam.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Prevent integration hooks re-trigger if server restart on config change, but the config failed to load
4 changes: 0 additions & 4 deletions packages/astro/src/core/dev/container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,3 @@ export async function startContainer({

return devServerAddressInfo;
}

export function isStarted(container: Container): boolean {
return !!container.viteServer.httpServer?.listening;
}
2 changes: 1 addition & 1 deletion packages/astro/src/core/dev/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export { createContainer, isStarted, startContainer } from './container.js';
export { createContainer, startContainer } from './container.js';
export { default } from './dev.js';
export { createContainerWithAutomaticRestart } from './restart.js';
41 changes: 17 additions & 24 deletions packages/astro/src/core/dev/restart.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,11 @@ import { createSafeError } from '../errors/index.js';
import { info, error as logError } from '../logger/core.js';
import { formatErrorMessage } from '../messages.js';
import type { Container } from './container';
import { createContainer, isStarted, startContainer } from './container.js';
import { createContainer, startContainer } from './container.js';

async function createRestartedContainer(
container: Container,
settings: AstroSettings,
needsStart: boolean
settings: AstroSettings
): Promise<Container> {
const { logging, fs, inlineConfig } = container;
const newContainer = await createContainer({
Expand All @@ -26,9 +25,7 @@ async function createRestartedContainer(
fs,
});

if (needsStart) {
await startContainer(newContainer);
}
await startContainer(newContainer);

return newContainer;
}
Expand Down Expand Up @@ -62,21 +59,15 @@ export function shouldRestartContainer(
return shouldRestart;
}

export async function restartContainer(
container: Container
): Promise<{ container: Container; error: Error | null }> {
export async function restartContainer(container: Container): Promise<Container | Error> {
const { logging, close, settings: existingSettings } = container;
container.restartInFlight = true;

const needsStart = isStarted(container);
try {
const { astroConfig } = await resolveConfig(container.inlineConfig, 'dev', container.fs);
const settings = createSettings(astroConfig, fileURLToPath(existingSettings.config.root));
await close();
return {
container: await createRestartedContainer(container, settings, needsStart),
error: null,
};
return await createRestartedContainer(container, settings);
} catch (_err) {
const error = createSafeError(_err);
// Print all error messages except ZodErrors from AstroConfig as the pre-logged error is sufficient
Expand All @@ -91,12 +82,9 @@ export async function restartContainer(
stack: error.stack || '',
},
});
await close();
container.restartInFlight = false;
info(logging, 'astro', 'Continuing with previous valid configuration\n');
return {
container: await createRestartedContainer(container, existingSettings, needsStart),
error,
};
return error;
}
}

Expand Down Expand Up @@ -137,11 +125,16 @@ export async function createContainerWithAutomaticRestart({
async function handleServerRestart(logMsg: string) {
info(logging, 'astro', logMsg + '\n');
const container = restart.container;
const { container: newContainer, error } = await restartContainer(container);
restart.container = newContainer;
// Add new watches because this is a new container with a new Vite server
addWatches();
resolveRestart(error);
const result = await restartContainer(container);
if (result instanceof Error) {
// Failed to restart, use existing container
resolveRestart(result);
} else {
// Restart success. Add new watches because this is a new container with a new Vite server
restart.container = result;
addWatches();
resolveRestart(null);
}
restartComplete = new Promise<Error | null>((resolve) => {
resolveRestart = resolve;
});
Expand Down
5 changes: 4 additions & 1 deletion packages/astro/test/units/dev/restart.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,16 @@ import { fileURLToPath } from 'node:url';

import {
createContainerWithAutomaticRestart,
isStarted,
startContainer,
} from '../../../dist/core/dev/index.js';
import { createFs, createRequestAndResponse, triggerFSEvent } from '../test-utils.js';

const root = new URL('../../fixtures/alias/', import.meta.url);

function isStarted(container) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@matthewp matthewp Aug 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

return !!container.viteServer.httpServer?.listening;
}

describe('dev container restarts', () => {
it('Surfaces config errors on restarts', async () => {
const fs = createFs(
Expand Down