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

fix: better handling of resync and restarts in content layer #12984

Merged
merged 3 commits into from
Jan 16, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/kind-horses-smile.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug in dev where files would stop being watched if the Astro config file was edited
5 changes: 5 additions & 0 deletions .changeset/large-cherries-explain.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug where the content layer would use an outdated version of the Astro config if it was edited in dev
18 changes: 15 additions & 3 deletions packages/astro/src/content/content-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
globalContentConfigObserver,
safeStringify,
} from './utils.js';
import { type WrappedWatcher, createWatcherWrapper } from './watcher.js';
import type { AstroConfig } from '../types/public/index.js';

Check warning on line 26 in packages/astro/src/content/content-layer.ts

View workflow job for this annotation

GitHub Actions / Lint

lint/correctness/noUnusedImports

This import is unused.

export interface ContentLayerOptions {
store: MutableDataStore;
Expand All @@ -30,11 +32,12 @@
watcher?: FSWatcher;
}


export class ContentLayer {
#logger: Logger;
#store: MutableDataStore;
#settings: AstroSettings;
#watcher?: FSWatcher;
#watcher?: WrappedWatcher;
#lastConfigDigest?: string;
#unsubscribe?: () => void;

Expand All @@ -49,7 +52,9 @@
this.#logger = logger;
this.#store = store;
this.#settings = settings;
this.#watcher = watcher;
if (watcher) {
this.#watcher = createWatcherWrapper(watcher);
}
this.#queue = new PQueue({ concurrency: 1 });
}

Expand Down Expand Up @@ -79,6 +84,7 @@
dispose() {
this.#queue.clear();
this.#unsubscribe?.();
this.#watcher?.removeAllTrackedListeners();
}

async #getGenerateDigest() {
Expand Down Expand Up @@ -178,7 +184,7 @@
} = this.#settings.config;

const astroConfigDigest = safeStringify(hashableConfig);

const { digest: currentConfigDigest } = contentConfig.config;
this.#lastConfigDigest = currentConfigDigest;

Expand Down Expand Up @@ -213,6 +219,12 @@
if (astroConfigDigest) {
await this.#store.metaStore().set('astro-config-digest', astroConfigDigest);
}

if (!options?.loaders?.length) {
// Remove all listeners before syncing, as they will be re-added by the loaders, but not if this is a selective sync
this.#watcher?.removeAllTrackedListeners();
}

await Promise.all(
Object.entries(contentConfig.config.collections).map(async ([name, collection]) => {
if (collection.type !== CONTENT_LAYER_TYPE) {
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/content/loaders/file.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ export function file(fileName: string, options?: FileOptions): Loader {

await syncData(filePath, context);

watcher?.add(filePath);

watcher?.on('change', async (changedPath) => {
if (changedPath === filePath) {
logger.info(`Reloading data from ${fileName}`);
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/content/loaders/glob.ts
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,8 @@ export function glob(globOptions: GlobOptions): Loader {
return;
}

watcher.add(filePath);

const matchesGlob = (entry: string) =>
!entry.startsWith('../') && micromatch.isMatch(entry, globOptions.pattern);

Expand Down
15 changes: 4 additions & 11 deletions packages/astro/src/content/server-listeners.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,7 @@ export async function attachContentServerListeners({
}: ContentServerListenerParams) {
const contentPaths = getContentPaths(settings.config, fs);
if (!settings.config.legacy?.collections) {
const contentGenerator = await createContentTypesGenerator({
fs,
settings,
logger,
viteServer,
contentConfigObserver: globalContentConfigObserver,
});
await contentGenerator.init();
await attachListeners();
} else if (fs.existsSync(contentPaths.contentDir)) {
logger.debug(
'content',
Expand Down Expand Up @@ -71,9 +64,9 @@ export async function attachContentServerListeners({
viteServer.watcher.on('addDir', (entry) =>
contentGenerator.queueEvent({ name: 'addDir', entry }),
);
viteServer.watcher.on('change', (entry) =>
contentGenerator.queueEvent({ name: 'change', entry }),
);
viteServer.watcher.on('change', (entry) => {
contentGenerator.queueEvent({ name: 'change', entry });
});
viteServer.watcher.on('unlink', (entry) => {
contentGenerator.queueEvent({ name: 'unlink', entry });
});
Expand Down
9 changes: 8 additions & 1 deletion packages/astro/src/content/types-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,14 @@ export async function createContentTypesGenerator({
entry: pathToFileURL(rawEvent.entry),
name: rawEvent.name,
};
if (!event.entry.pathname.startsWith(contentPaths.contentDir.pathname)) return;

if (settings.config.legacy.collections) {
if (!event.entry.pathname.startsWith(contentPaths.contentDir.pathname)) {
return;
}
} else if (contentPaths.config.url.pathname !== event.entry.pathname) {
return;
}

events.push(event);

Expand Down
62 changes: 62 additions & 0 deletions packages/astro/src/content/watcher.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import type { FSWatcher } from 'vite';

type WatchEventName = 'add' | 'addDir' | 'change' | 'unlink' | 'unlinkDir';
type WatchEventCallback = (path: string) => void;

export type WrappedWatcher = FSWatcher & {
removeAllTrackedListeners(): void;
};

// This lets us use the standard Vite FSWatcher, but also track all listeners added by the content loaders
// We do this so we can remove them all when we re-sync.
export function createWatcherWrapper(watcher: FSWatcher): WrappedWatcher {
const listeners = new Map<WatchEventName, Set<WatchEventCallback>>();

const handler: ProxyHandler<FSWatcher> = {
Copy link
Member

@florian-lefebvre florian-lefebvre Jan 15, 2025

Choose a reason for hiding this comment

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

Isn't a proxy overkill for this? Could it be an object?

return {
	on: (event, callback) => {
				 if (!listeners.has(event)) {
						listeners.set(event, new Set());
					}

					// Track the listener
					listeners.get(event)!.add(callback);

				return watcher.on(event, callback)
	},
	// off, etc
}

Or is it because you want to support all other methods too?

Copy link
Contributor Author

@ascorbic ascorbic Jan 15, 2025

Choose a reason for hiding this comment

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

Yeah, it's a public API, so it would be a breaking change if I changed the type.

get(target, prop, receiver) {
// Intercept the 'on' method and track the listener
if (prop === 'on') {
return function (event: WatchEventName, callback: WatchEventCallback) {
if (!listeners.has(event)) {
listeners.set(event, new Set());
}

// Track the listener
listeners.get(event)!.add(callback);

// Call the original method
return Reflect.get(target, prop, receiver).call(target, event, callback);
};
}

// Intercept the 'off' method
if (prop === 'off') {
return function (event: WatchEventName, callback: WatchEventCallback) {
// Remove from our tracking
listeners.get(event)?.delete(callback);

// Call the original method
return Reflect.get(target, prop, receiver).call(target, event, callback);
};
}

// Adds a function to remove all listeners added by us
if (prop === 'removeAllTrackedListeners') {
return function () {
for (const [event, callbacks] of listeners.entries()) {
for (const callback of callbacks) {
target.off(event, callback);
}
callbacks.clear();
}
listeners.clear();
};
}

// Return original method/property for everything else
return Reflect.get(target, prop, receiver);
},
};

return new Proxy(watcher, handler) as WrappedWatcher;
}
5 changes: 3 additions & 2 deletions packages/astro/src/core/dev/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -108,21 +108,22 @@ export async function createContainer({
ssrManifest: devSSRManifest,
},
);
const viteServer = await vite.createServer(viteConfig);

await syncInternal({
settings,
mode,
logger,
skip: {
content: true,
content: !isRestart,
cleanup: true,
},
force: inlineConfig?.force,
manifest,
command: 'dev',
watcher: viteServer.watcher,
});

const viteServer = await vite.createServer(viteConfig);

const container: Container = {
inlineConfig: inlineConfig ?? {},
Expand Down
3 changes: 3 additions & 0 deletions packages/astro/src/core/dev/restart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { createSafeError } from '../errors/index.js';
import { formatErrorMessage } from '../messages.js';
import type { Container } from './container.js';
import { createContainer, startContainer } from './container.js';
import { attachContentServerListeners } from '../../content/server-listeners.js';

async function createRestartedContainer(
container: Container,
Expand Down Expand Up @@ -147,6 +148,8 @@ export async function createContainerWithAutomaticRestart({
// Restart success. Add new watches because this is a new container with a new Vite server
restart.container = result;
setupContainer();
await attachContentServerListeners(restart.container);

if (server) {
// Vite expects the resolved URLs to be available
server.resolvedUrls = result.viteServer.resolvedUrls;
Expand Down
10 changes: 9 additions & 1 deletion packages/astro/src/core/sync/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { dirname, relative } from 'node:path';
import { performance } from 'node:perf_hooks';
import { fileURLToPath } from 'node:url';
import { dim } from 'kleur/colors';
import { type HMRPayload, createServer } from 'vite';
import { type FSWatcher, type HMRPayload, createServer } from 'vite';
import { CONTENT_TYPES_FILE } from '../../content/consts.js';
import { getDataStoreFile, globalContentLayer } from '../../content/content-layer.js';
import { createContentTypesGenerator } from '../../content/index.js';
Expand Down Expand Up @@ -50,6 +50,7 @@ export type SyncOptions = {
};
manifest: ManifestData;
command: 'build' | 'dev' | 'sync';
watcher?: FSWatcher;
};

export default async function sync(
Expand Down Expand Up @@ -120,6 +121,7 @@ export async function syncInternal({
force,
manifest,
command,
watcher,
}: SyncOptions): Promise<void> {
const isDev = command === 'dev';
if (force) {
Expand All @@ -131,6 +133,7 @@ export async function syncInternal({
if (!skip?.content) {
await syncContentCollections(settings, { mode, fs, logger, manifest });
settings.timer.start('Sync content layer');

let store: MutableDataStore | undefined;
try {
const dataStoreFile = getDataStoreFile(settings, isDev);
Expand All @@ -142,11 +145,16 @@ export async function syncInternal({
logger.error('content', 'Failed to load content store');
return;
}

const contentLayer = globalContentLayer.init({
settings,
logger,
store,
watcher,
});
if (watcher) {
contentLayer.watchContentConfig();
}
await contentLayer.sync();
if (!skip?.cleanup) {
// Free up memory (usually in builds since we only need to use this once)
Expand Down
33 changes: 32 additions & 1 deletion packages/astro/test/content-layer.test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import assert from 'node:assert/strict';
import { promises as fs, existsSync } from 'node:fs';
import { setTimeout } from 'node:timers/promises';
import { sep } from 'node:path';
import { sep as posixSep } from 'node:path/posix';
import { Writable } from 'node:stream';
Expand Down Expand Up @@ -323,7 +324,7 @@ describe('Content Layer', () => {
devServer = await fixture.startDevServer({
force: true,
logger: new Logger({
level: 'warn',
level: 'info',
dest: new Writable({
objectMode: true,
write(event, _, callback) {
Expand Down Expand Up @@ -524,5 +525,35 @@ describe('Content Layer', () => {
await fs.rename(newPath, oldPath);
}
});

it('still updates collection when data file is changed after server has restarted via config change', async () => {
await fixture.editFile('astro.config.mjs', (prev) =>
prev.replace("'Astro content layer'", "'Astro content layer edited'"),
);
logs.length = 0;

// Give time for the server to restart
await setTimeout(5000);

const rawJsonResponse = await fixture.fetch('/collections.json');
const initialJson = devalue.parse(await rawJsonResponse.text());
assert.equal(initialJson.jsonLoader[0].data.temperament.includes('Bouncy'), false);

await fixture.editFile('/src/data/dogs.json', (prev) => {
const data = JSON.parse(prev);
data[0].temperament.push('Bouncy');
return JSON.stringify(data, null, 2);
});

await fixture.onNextDataStoreChange();
const updatedJsonResponse = await fixture.fetch('/collections.json');
const updated = devalue.parse(await updatedJsonResponse.text());
assert.ok(updated.jsonLoader[0].data.temperament.includes('Bouncy'));
logs.length = 0;

await fixture.resetAllFiles();
// Give time for the server to restart again
await setTimeout(5000);
});
});
});
4 changes: 2 additions & 2 deletions packages/markdown/remark/src/import-plugin-default.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ let cwdUrlStr: string | undefined;
export async function importPlugin(p: string): Promise<unified.Plugin> {
// Try import from this package first
try {
const importResult = await import(p);
const importResult = await import(/* @vite-ignore */ p);
return importResult.default;
} catch {}

// Try import from user project
cwdUrlStr ??= pathToFileURL(path.join(process.cwd(), 'package.json')).toString();
const resolved = importMetaResolve(p, cwdUrlStr);
const importResult = await import(resolved);
const importResult = await import(/* @vite-ignore */ resolved);
return importResult.default;
}
Loading