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

Conversation

ascorbic
Copy link
Contributor

@ascorbic ascorbic commented Jan 14, 2025

Changes

This PR makes a number of changes to the way that file watchers work in dev with the content layer. There have been a few bugs related to the way that dev server restarts are handled, which were due to the way we were handling file watchers.

When the Astro config is edited, it restarts the dev container to ensure the site is using the latest config. This was causing several bugs:

  • when the server was restarted, it wasn't re-running the Astro sync. This meant that updates in the config were not reflected in the rendered config, for example changes to Markdown config wouldn't update the rendered content.
  • the existing watcher was destroyed, but because the loaders weren't being re-run, the glob and file event listeners weren't being attached to the new watcher.

This PR correctly passes in the watcher, but has to handle various implications of this:

  • it needs to ensure that the content layer is recreated when the server is restarted, otherwise it will use the old config even when it syncs the content. This fixes Shiki Theme Changes Not Reflected in Content Collection #12700
  • it needs to detach the event listeners that the previous loader run added. We can't clear them all, because this is a shared watcher instance. To handle this, we now wrap the watcher with a Proxy that keeps track of which listeners were attached by the content layer, so they can be removed if the content layer is refreshed or disposed of. Fixes content collections stop updating in dev server after restarting for Astro config updates #12976
  • it needs to ensure that the files themselves are still being watched, so it now manually adds them to the watcher rather than relying on them still being watched

Testing

Added new test cases

Docs

Copy link

changeset-bot bot commented Jan 14, 2025

🦋 Changeset detected

Latest commit: 8452a0d

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 github-actions bot added feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope) labels Jan 14, 2025
Copy link

codspeed-hq bot commented Jan 14, 2025

CodSpeed Performance Report

Merging #12984 will not alter performance

Comparing watch-after-reload (8452a0d) with main (1a026af)

Summary

✅ 6 untouched benchmarks

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.

@ascorbic ascorbic merged commit 2d259cf into main Jan 16, 2025
16 checks passed
@ascorbic ascorbic deleted the watch-after-reload branch January 16, 2025 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: markdown Related to Markdown (scope) pkg: astro Related to the core `astro` package (scope)
Projects
None yet
3 participants