From 1539e04a8e5865027b3a8718c6f142885e7c8d88 Mon Sep 17 00:00:00 2001 From: Bjorn Lu Date: Wed, 17 Jan 2024 11:27:21 +0800 Subject: [PATCH] Simplify HMR for circular imports and CSS (#9706) --- .changeset/curvy-seas-explain.md | 5 + .changeset/tidy-planets-whisper.md | 5 + .../e2e/fixtures/tailwindcss/astro.config.mjs | 1 + .../tailwindcss/src/components/Layout.astro | 16 ++++ .../tailwindcss/src/pages/index.astro | 17 +--- packages/astro/src/core/logger/vite.ts | 2 +- .../astro/src/vite-plugin-astro/compile.ts | 5 - packages/astro/src/vite-plugin-astro/hmr.ts | 95 +++++++------------ packages/astro/src/vite-plugin-astro/index.ts | 41 +++++++- packages/astro/src/vite-plugin-astro/query.ts | 5 - .../units/vite-plugin-astro/compile.test.js | 5 - packages/integrations/mdx/src/index.ts | 16 +--- 12 files changed, 110 insertions(+), 103 deletions(-) create mode 100644 .changeset/curvy-seas-explain.md create mode 100644 .changeset/tidy-planets-whisper.md create mode 100644 packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro diff --git a/.changeset/curvy-seas-explain.md b/.changeset/curvy-seas-explain.md new file mode 100644 index 000000000000..debdb45a92e6 --- /dev/null +++ b/.changeset/curvy-seas-explain.md @@ -0,0 +1,5 @@ +--- +"@astrojs/mdx": patch +--- + +Removes redundant HMR handling code diff --git a/.changeset/tidy-planets-whisper.md b/.changeset/tidy-planets-whisper.md new file mode 100644 index 000000000000..885fb157742b --- /dev/null +++ b/.changeset/tidy-planets-whisper.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Simplifies HMR handling, improves circular dependency invalidation, and fixes Astro styles invalidation diff --git a/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs b/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs index aa1ca234a34b..f0899cd91dd4 100644 --- a/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs +++ b/packages/astro/e2e/fixtures/tailwindcss/astro.config.mjs @@ -7,6 +7,7 @@ export default defineConfig({ integrations: [ tailwind({ configFile: fileURLToPath(new URL('./tailwind.config.js', import.meta.url)), + applyBaseStyles: false }), ], devToolbar: { diff --git a/packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro b/packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro new file mode 100644 index 000000000000..583809b5605d --- /dev/null +++ b/packages/astro/e2e/fixtures/tailwindcss/src/components/Layout.astro @@ -0,0 +1,16 @@ + + + + + Astro + TailwindCSS + + + + + + + diff --git a/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro b/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro index d901b4233a9c..948fa51df036 100644 --- a/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro +++ b/packages/astro/e2e/fixtures/tailwindcss/src/pages/index.astro @@ -1,18 +1,11 @@ --- // Component Imports +import Layout from '../components/Layout.astro'; import Button from '../components/Button.astro'; import Complex from '../components/Complex.astro'; --- - - - - - Astro + TailwindCSS - - - - - - - + + + + diff --git a/packages/astro/src/core/logger/vite.ts b/packages/astro/src/core/logger/vite.ts index ac48369a3693..8a3079158b55 100644 --- a/packages/astro/src/core/logger/vite.ts +++ b/packages/astro/src/core/logger/vite.ts @@ -6,7 +6,7 @@ import { isLogLevelEnabled, type Logger as AstroLogger } from './core.js'; const PKG_PREFIX = fileURLToPath(new URL('../../../', import.meta.url)); const E2E_PREFIX = fileURLToPath(new URL('../../../e2e', import.meta.url)); -export function isAstroSrcFile(id: string | null) { +function isAstroSrcFile(id: string | null) { return id?.startsWith(PKG_PREFIX) && !id.startsWith(E2E_PREFIX); } diff --git a/packages/astro/src/vite-plugin-astro/compile.ts b/packages/astro/src/vite-plugin-astro/compile.ts index 768d18d86832..85c2d97fc38a 100644 --- a/packages/astro/src/vite-plugin-astro/compile.ts +++ b/packages/astro/src/vite-plugin-astro/compile.ts @@ -76,11 +76,6 @@ export async function cachedFullCompilation({ } } - // Prefer live reload to HMR in `.astro` files - if (!compileProps.viteConfig.isProduction) { - SUFFIX += `\nif (import.meta.hot) { import.meta.hot.decline() }`; - } - return { ...transformResult, code: esbuildResult.code + SUFFIX, diff --git a/packages/astro/src/vite-plugin-astro/hmr.ts b/packages/astro/src/vite-plugin-astro/hmr.ts index d06a8338f662..b7e1dc48b778 100644 --- a/packages/astro/src/vite-plugin-astro/hmr.ts +++ b/packages/astro/src/vite-plugin-astro/hmr.ts @@ -1,14 +1,15 @@ -import type { HmrContext, ModuleNode } from 'vite'; +import path from 'node:path'; +import { appendForwardSlash } from '@astrojs/internal-helpers/path'; +import type { HmrContext } from 'vite'; import type { AstroConfig } from '../@types/astro.js'; import type { cachedCompilation } from '../core/compile/index.js'; import { invalidateCompilation, isCached, type CompileResult } from '../core/compile/index.js'; import type { Logger } from '../core/logger/core.js'; -import { isAstroSrcFile } from '../core/logger/vite.js'; -import { isAstroScript } from './query.js'; export interface HandleHotUpdateOptions { config: AstroConfig; logger: Logger; + astroFileToCssAstroDeps: Map>; compile: () => ReturnType; source: string; @@ -16,7 +17,7 @@ export interface HandleHotUpdateOptions { export async function handleHotUpdate( ctx: HmrContext, - { config, logger, compile, source }: HandleHotUpdateOptions + { config, logger, astroFileToCssAstroDeps, compile, source }: HandleHotUpdateOptions ) { let isStyleOnlyChange = false; if (ctx.file.endsWith('.astro') && isCached(config, ctx.file)) { @@ -34,75 +35,45 @@ export async function handleHotUpdate( invalidateCompilation(config, ctx.file); } - // Skip monorepo files to avoid console spam - if (isAstroSrcFile(ctx.file)) { - return; - } - - // go through each of these modules importers and invalidate any .astro compilation - // that needs to be rerun. - const filtered = new Set(ctx.modules); - const files = new Set(); - for (const mod of ctx.modules) { - // Skip monorepo files to avoid console spam - if (isAstroSrcFile(mod.id ?? mod.file)) { - filtered.delete(mod); - continue; - } - - if (mod.file && isCached(config, mod.file)) { - filtered.add(mod); - files.add(mod.file); - } - - for (const imp of mod.importers) { - if (imp.file && isCached(config, imp.file)) { - filtered.add(imp); - files.add(imp.file); - } - } - } - - // Invalidate happens as a separate step because a single .astro file - // produces multiple CSS modules and we want to return all of those. - for (const file of files) { - if (isStyleOnlyChange && file === ctx.file) continue; - invalidateCompilation(config, file); - // If `ctx.file` is depended by an .astro file, e.g. via `this.addWatchFile`, - // Vite doesn't trigger updating that .astro file by default. See: - // https://github.com/vitejs/vite/issues/3216 - // For now, we trigger the change manually here. - if (file.endsWith('.astro')) { - ctx.server.moduleGraph.onFileChange(file); - } - } - - // Bugfix: sometimes style URLs get normalized and end with `lang.css=` - // These will cause full reloads, so filter them out here - const mods = [...filtered].filter((m) => !m.url.endsWith('=')); - - // If only styles are changed, remove the component file from the update list if (isStyleOnlyChange) { logger.debug('watch', 'style-only change'); // Only return the Astro styles that have changed! - return mods.filter((mod) => mod.id?.includes('astro&type=style')); + return ctx.modules.filter((mod) => mod.id?.includes('astro&type=style')); } - // Add hoisted scripts so these get invalidated - for (const mod of mods) { - for (const imp of mod.importedModules) { - if (imp.id && isAstroScript(imp.id)) { - mods.push(imp); + // Edge case handling usually caused by Tailwind creating circular dependencies + // + // TODO: we can also workaround this with better CSS dependency management for Astro files, + // so that changes within style tags are scoped to itself. But it'll take a bit of work. + // https://github.com/withastro/astro/issues/9370#issuecomment-1850160421 + for (const [astroFile, cssAstroDeps] of astroFileToCssAstroDeps) { + // If the `astroFile` has a CSS dependency on `ctx.file`, there's a good chance this causes a + // circular dependency, which Vite doesn't issue a full page reload. Workaround it by forcing a + // full page reload ourselves. (Vite bug) + // https://github.com/vitejs/vite/pull/15585 + if (cssAstroDeps.has(ctx.file)) { + // Mimic the HMR log as if this file is updated + logger.info('watch', getShortName(ctx.file, ctx.server.config.root)); + // Invalidate the modules of `astroFile` explicitly as Vite may incorrectly soft-invalidate + // the parent if the parent actually imported `ctx.file`, but `this.addWatchFile` was also called + // on `ctx.file`. Vite should do a hard-invalidation instead. (Vite bug) + const parentModules = ctx.server.moduleGraph.getModulesByFile(astroFile); + if (parentModules) { + for (const mod of parentModules) { + ctx.server.moduleGraph.invalidateModule(mod); + } } + ctx.server.ws.send({ type: 'full-reload', path: '*' }); } } - - return mods; } function isStyleOnlyChanged(oldResult: CompileResult, newResult: CompileResult) { return ( normalizeCode(oldResult.code) === normalizeCode(newResult.code) && + // If style tags are added/removed, we need to regenerate the main Astro file + // so that its CSS imports are also added/removed + oldResult.css.length === newResult.css.length && !isArrayEqual(oldResult.css, newResult.css) ); } @@ -129,3 +100,7 @@ function isArrayEqual(a: any[], b: any[]) { } return true; } + +function getShortName(file: string, root: string): string { + return file.startsWith(appendForwardSlash(root)) ? path.posix.relative(root, file) : file; +} diff --git a/packages/astro/src/vite-plugin-astro/index.ts b/packages/astro/src/vite-plugin-astro/index.ts index 6fe53cdc81f7..4a5009146864 100644 --- a/packages/astro/src/vite-plugin-astro/index.ts +++ b/packages/astro/src/vite-plugin-astro/index.ts @@ -9,6 +9,7 @@ import { cachedCompilation, getCachedCompileResult, type CompileProps, + invalidateCompilation, } from '../core/compile/index.js'; import { isRelativePath } from '../core/path.js'; import { normalizeFilename } from '../vite-plugin-utils/index.js'; @@ -27,7 +28,13 @@ interface AstroPluginOptions { export default function astro({ settings, logger }: AstroPluginOptions): vite.Plugin[] { const { config } = settings; let resolvedConfig: vite.ResolvedConfig; - let server: vite.ViteDevServer; + let server: vite.ViteDevServer | undefined; + // Tailwind styles could register Astro files as dependencies of other Astro files, + // causing circular imports which trips Vite's HMR. This set is passed to `handleHotUpdate` + // to force a page reload when these dependency files are updated + // NOTE: We need to initialize a map here and in `buildStart` because our unit tests don't + // call `buildStart` (test bug) + let astroFileToCssAstroDeps = new Map>(); // Variables for determining if an id starts with /src... const srcRootWeb = config.srcDir.pathname.slice(config.root.pathname.length - 1); @@ -42,6 +49,9 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl configureServer(_server) { server = _server; }, + buildStart() { + astroFileToCssAstroDeps = new Map(); + }, async load(id, opts) { const parsedId = parseAstroRequest(id); const query = parsedId.query; @@ -157,11 +167,39 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl source, }; + // We invalidate and then compile again as we know Vite will only call this `transform` + // when its cache is invalidated. + // TODO: Do the compilation directly and remove our cache so we rely on Vite only. + invalidateCompilation(config, compileProps.filename); const transformResult = await cachedFullCompilation({ compileProps, logger }); + // Register dependencies of this module + const astroDeps = new Set(); for (const dep of transformResult.cssDeps) { + if (dep.endsWith('.astro')) { + astroDeps.add(dep); + } this.addWatchFile(dep); } + astroFileToCssAstroDeps.set(id, astroDeps); + + // When a dependency from the styles are updated, the dep and Astro module will get invalidated. + // However, the Astro style virtual module is not invalidated because we didn't register that the virtual + // module has that dependency. We currently can't do that either because of a Vite bug. + // https://github.com/vitejs/vite/pull/15608 + // Here we manually invalidate the virtual modules ourselves when we're compiling the Astro module. + // When that bug is resolved, we can add the dependencies to the virtual module directly and remove this. + if (server) { + const mods = server.moduleGraph.getModulesByFile(compileProps.filename); + if (mods) { + const seen = new Set(mods); + for (const mod of mods) { + if (mod.url.includes('?astro')) { + server.moduleGraph.invalidateModule(mod, seen); + } + } + } + } const astroMetadata: AstroPluginMetadata['astro'] = { clientOnlyComponents: transformResult.clientOnlyComponents, @@ -200,6 +238,7 @@ export default function astro({ settings, logger }: AstroPluginOptions): vite.Pl return handleHotUpdate(context, { config, logger, + astroFileToCssAstroDeps, compile, source, }); diff --git a/packages/astro/src/vite-plugin-astro/query.ts b/packages/astro/src/vite-plugin-astro/query.ts index a0857fdfed61..c9829de3f6a8 100644 --- a/packages/astro/src/vite-plugin-astro/query.ts +++ b/packages/astro/src/vite-plugin-astro/query.ts @@ -35,8 +35,3 @@ export function parseAstroRequest(id: string): ParsedRequestResult { query, }; } - -export function isAstroScript(id: string): boolean { - const parsed = parseAstroRequest(id); - return parsed.query.type === 'script'; -} diff --git a/packages/astro/test/units/vite-plugin-astro/compile.test.js b/packages/astro/test/units/vite-plugin-astro/compile.test.js index 5fa87433eb90..4427d3acd01a 100644 --- a/packages/astro/test/units/vite-plugin-astro/compile.test.js +++ b/packages/astro/test/units/vite-plugin-astro/compile.test.js @@ -62,11 +62,6 @@ const name = 'world expect(result).to.be.undefined; }); - it('injects hmr code', async () => { - const result = await compile(`

Hello World

`, '/src/components/index.astro'); - expect(result.code).to.include('import.meta.hot'); - }); - it('has file and url exports for markdwon compat', async () => { const result = await compile(`

Hello World

`, '/src/components/index.astro'); await init; diff --git a/packages/integrations/mdx/src/index.ts b/packages/integrations/mdx/src/index.ts index 1b38da48820e..78c8ce889ec7 100644 --- a/packages/integrations/mdx/src/index.ts +++ b/packages/integrations/mdx/src/index.ts @@ -40,14 +40,8 @@ export default function mdx(partialMdxOptions: Partial = {}): AstroI name: '@astrojs/mdx', hooks: { 'astro:config:setup': async (params) => { - const { - updateConfig, - config, - addPageExtension, - addContentEntryType, - command, - addRenderer, - } = params as SetupHookParams; + const { updateConfig, config, addPageExtension, addContentEntryType, addRenderer } = + params as SetupHookParams; addRenderer(astroJSXRenderer); addPageExtension('.mdx'); @@ -209,12 +203,6 @@ export default function mdx(partialMdxOptions: Partial = {}): AstroI code += `\nContent[Symbol.for('astro.needsHeadRendering')] = !Boolean(frontmatter.layout);`; code += `\nContent.moduleId = ${JSON.stringify(id)};`; - if (command === 'dev') { - // TODO: decline HMR updates until we have a stable approach - code += `\nif (import.meta.hot) { - import.meta.hot.decline(); - }`; - } return { code, map: null }; }, },