From b2cc703e4485668685bb24f8fd7411a212672281 Mon Sep 17 00:00:00 2001 From: Romuald Brillout Date: Fri, 1 Apr 2022 11:11:28 +0200 Subject: [PATCH 1/4] fix: fix HMR propagation when imports not analyzed --- packages/playground/hmr/__tests__/hmr.spec.ts | 29 +++++++++++++++++++ packages/playground/hmr/dynamic-import/dep.ts | 1 + .../playground/hmr/dynamic-import/index.html | 2 ++ .../playground/hmr/dynamic-import/index.ts | 12 ++++++++ .../vite/src/node/plugins/importAnalysis.ts | 21 +++++++++----- packages/vite/src/node/server/hmr.ts | 6 ++++ packages/vite/src/node/server/moduleGraph.ts | 2 +- 7 files changed, 64 insertions(+), 9 deletions(-) create mode 100644 packages/playground/hmr/dynamic-import/dep.ts create mode 100644 packages/playground/hmr/dynamic-import/index.html create mode 100644 packages/playground/hmr/dynamic-import/index.ts diff --git a/packages/playground/hmr/__tests__/hmr.spec.ts b/packages/playground/hmr/__tests__/hmr.spec.ts index 6ddc2345ae4fb4..4f3774cf70502a 100644 --- a/packages/playground/hmr/__tests__/hmr.spec.ts +++ b/packages/playground/hmr/__tests__/hmr.spec.ts @@ -160,4 +160,33 @@ if (!isBuild) { expect(textprev).not.toMatch('direct') expect(textpost).not.toMatch('direct') }) + + test('not loaded dynamic import', async () => { + await page.goto(viteTestUrl + '/dynamic-import/index.html') + + let btn = await page.$('button') + expect(await btn.textContent()).toBe('Counter 0') + await btn.click() + expect(await btn.textContent()).toBe('Counter 1') + + // Modifying `index.ts` triggers a page reload, as expected + editFile('dynamic-import/index.ts', (code) => code) + await page.waitForNavigation() + btn = await page.$('button') + expect(await btn.textContent()).toBe('Counter 0') + + await btn.click() + expect(await btn.textContent()).toBe('Counter 1') + + // Modifying a dynamic import that has not been loaded has no effect (doesn't trigger a page reload) + editFile('dynamic-import/dep.ts', (code) => code) + try { + await page.waitForNavigation({ timeout: 1000 }) + } catch (err) { + const errMsg = 'page.waitForNavigation: Timeout 1000ms exceeded.' + expect(err.message.slice(0, errMsg.length)).toBe(errMsg) + } + btn = await page.$('button') + expect(await btn.textContent()).toBe('Counter 1') + }) } diff --git a/packages/playground/hmr/dynamic-import/dep.ts b/packages/playground/hmr/dynamic-import/dep.ts new file mode 100644 index 00000000000000..d211b19b6a4a73 --- /dev/null +++ b/packages/playground/hmr/dynamic-import/dep.ts @@ -0,0 +1 @@ +// This file is never loaded diff --git a/packages/playground/hmr/dynamic-import/index.html b/packages/playground/hmr/dynamic-import/index.html new file mode 100644 index 00000000000000..f5290ad4f1e507 --- /dev/null +++ b/packages/playground/hmr/dynamic-import/index.html @@ -0,0 +1,2 @@ + + diff --git a/packages/playground/hmr/dynamic-import/index.ts b/packages/playground/hmr/dynamic-import/index.ts new file mode 100644 index 00000000000000..0230140278989f --- /dev/null +++ b/packages/playground/hmr/dynamic-import/index.ts @@ -0,0 +1,12 @@ +const btn = document.querySelector('button') +let count = 0 +const update = () => { + btn.textContent = `Counter ${count}` +} +btn.onclick = () => { + count++ + update() +} +function neverCalled() { + import('./dep') +} diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index f6d6c410411712..ca1400845eb5bc 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -112,7 +112,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { tryIndex: false, extensions: [] }) - let server: ViteDevServer + let server: ViteDevServer | null = null return { name: 'vite:import-analysis', @@ -122,6 +122,10 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { }, async transform(source, importer, options) { + if (server === null) { + return null + } + const ssr = options?.ssr === true const prettyImporter = prettifyUrl(importer, root) @@ -159,7 +163,13 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { ) } + const { moduleGraph } = server + // since we are already in the transform phase of the importer, it must + // have been loaded so its entry is guaranteed in the module graph. + const importerModule = moduleGraph.getModuleById(importer)! + if (!imports.length) { + importerModule.isSelfAccepting = false isDebug && debug( `${timeFrom(start)} ${colors.dim(`[no imports] ${prettyImporter}`)}` @@ -173,11 +183,6 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { let needQueryInjectHelper = false let s: MagicString | undefined const str = () => s || (s = new MagicString(source)) - // vite-only server context - const { moduleGraph } = server - // since we are already in the transform phase of the importer, it must - // have been loaded so its entry is guaranteed in the module graph. - const importerModule = moduleGraph.getModuleById(importer)! const importedUrls = new Set() const staticImportedUrls = new Set() const acceptedUrls = new Set<{ @@ -198,7 +203,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { let importerFile = importer if (moduleListContains(config.optimizeDeps?.exclude, url)) { - const optimizedDeps = server._optimizedDeps + const optimizedDeps = server!._optimizedDeps if (optimizedDeps) { await optimizedDeps.scanProcessing @@ -659,7 +664,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { NULL_BYTE_PLACEHOLDER, '\0' ) - transformRequest(url, server, { ssr }).catch((e) => { + transformRequest(url, server!, { ssr }).catch((e) => { if (e?.code === ERR_OUTDATED_OPTIMIZED_DEP) { // This are expected errors return diff --git a/packages/vite/src/node/server/hmr.ts b/packages/vite/src/node/server/hmr.ts index fc18b0aa91c5cb..60a1f42508725f 100644 --- a/packages/vite/src/node/server/hmr.ts +++ b/packages/vite/src/node/server/hmr.ts @@ -225,6 +225,12 @@ function propagateUpdate( }>, currentChain: ModuleNode[] = [node] ): boolean /* hasDeadEnd */ { + // if the imports of `node` have not been analyzed, then `node` has not + // been loaded in the browser and we should stop propagation. + if (node.id && node.isSelfAccepting === null) { + return false + } + if (node.isSelfAccepting) { boundaries.add({ boundary: node, diff --git a/packages/vite/src/node/server/moduleGraph.ts b/packages/vite/src/node/server/moduleGraph.ts index 44e76deef98b6f..6a2c3deafdb0ca 100644 --- a/packages/vite/src/node/server/moduleGraph.ts +++ b/packages/vite/src/node/server/moduleGraph.ts @@ -27,7 +27,7 @@ export class ModuleNode { importers = new Set() importedModules = new Set() acceptedHmrDeps = new Set() - isSelfAccepting = false + isSelfAccepting: boolean | null = null transformResult: TransformResult | null = null ssrTransformResult: TransformResult | null = null ssrModule: Record | null = null From 50b7b1d6d8d5b8bb38b1e7232faca1c16326d123 Mon Sep 17 00:00:00 2001 From: Romuald Brillout Date: Fri, 1 Apr 2022 21:13:13 +0200 Subject: [PATCH 2/4] fix: misc --- packages/playground/hmr/__tests__/hmr.spec.ts | 7 ++++++- packages/playground/hmr/dynamic-import/dep.ts | 1 + packages/vite/src/node/plugins/importAnalysis.ts | 2 ++ packages/vite/src/node/server/hmr.ts | 3 ++- packages/vite/src/node/server/moduleGraph.ts | 2 +- 5 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/playground/hmr/__tests__/hmr.spec.ts b/packages/playground/hmr/__tests__/hmr.spec.ts index 4f3774cf70502a..7325c9fe47943a 100644 --- a/packages/playground/hmr/__tests__/hmr.spec.ts +++ b/packages/playground/hmr/__tests__/hmr.spec.ts @@ -178,7 +178,12 @@ if (!isBuild) { await btn.click() expect(await btn.textContent()).toBe('Counter 1') - // Modifying a dynamic import that has not been loaded has no effect (doesn't trigger a page reload) + // #7561 + // `dep.ts` defines `import.module.hot.accept` and has not been loaded. + // Therefore, modifying it has no effect (doesn't trigger a page reload). + // (Note that, a dynamic import that is never loaded and that does not + // define `accept.module.hot.accept` may wrongfully trigger a full page + // reload, see discussion at #7561.) editFile('dynamic-import/dep.ts', (code) => code) try { await page.waitForNavigation({ timeout: 1000 }) diff --git a/packages/playground/hmr/dynamic-import/dep.ts b/packages/playground/hmr/dynamic-import/dep.ts index d211b19b6a4a73..aca649f226f770 100644 --- a/packages/playground/hmr/dynamic-import/dep.ts +++ b/packages/playground/hmr/dynamic-import/dep.ts @@ -1 +1,2 @@ // This file is never loaded +import.meta.hot.accept(() => {}) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index ca1400845eb5bc..85a31426f0a3f5 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -122,6 +122,8 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { }, async transform(source, importer, options) { + // In a real app `server` is always defined, but it can be `null` when + // running the test https://github.com/vitejs/vite/blob/a74bd7ba9947be193bccf636b8918bd1f59e89ae/packages/vite/src/node/server/__tests__/pluginContainer.spec.ts if (server === null) { return null } diff --git a/packages/vite/src/node/server/hmr.ts b/packages/vite/src/node/server/hmr.ts index 60a1f42508725f..d80ba3f997bf7b 100644 --- a/packages/vite/src/node/server/hmr.ts +++ b/packages/vite/src/node/server/hmr.ts @@ -225,9 +225,10 @@ function propagateUpdate( }>, currentChain: ModuleNode[] = [node] ): boolean /* hasDeadEnd */ { + // #7561 // if the imports of `node` have not been analyzed, then `node` has not // been loaded in the browser and we should stop propagation. - if (node.id && node.isSelfAccepting === null) { + if (node.id && node.isSelfAccepting === undefined) { return false } diff --git a/packages/vite/src/node/server/moduleGraph.ts b/packages/vite/src/node/server/moduleGraph.ts index 6a2c3deafdb0ca..e470fafb05d8fd 100644 --- a/packages/vite/src/node/server/moduleGraph.ts +++ b/packages/vite/src/node/server/moduleGraph.ts @@ -27,7 +27,7 @@ export class ModuleNode { importers = new Set() importedModules = new Set() acceptedHmrDeps = new Set() - isSelfAccepting: boolean | null = null + isSelfAccepting?: boolean transformResult: TransformResult | null = null ssrTransformResult: TransformResult | null = null ssrModule: Record | null = null From d88d08cd49d1a229e05721b83384b43702f1758a Mon Sep 17 00:00:00 2001 From: patak Date: Sat, 2 Apr 2022 00:08:56 +0200 Subject: [PATCH 3/4] chore: update comment --- packages/vite/src/node/plugins/importAnalysis.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index 85a31426f0a3f5..f9fabbbfcdd298 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -123,7 +123,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { async transform(source, importer, options) { // In a real app `server` is always defined, but it can be `null` when - // running the test https://github.com/vitejs/vite/blob/a74bd7ba9947be193bccf636b8918bd1f59e89ae/packages/vite/src/node/server/__tests__/pluginContainer.spec.ts + // running src/node/server/__tests__/pluginContainer.spec.ts if (server === null) { return null } From fa4fa6d6562185d8d017147ca86642cff2b510dc Mon Sep 17 00:00:00 2001 From: patak-dev Date: Sat, 2 Apr 2022 00:18:06 +0200 Subject: [PATCH 4/4] chore: avoid some changes --- packages/vite/src/node/plugins/importAnalysis.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/vite/src/node/plugins/importAnalysis.ts b/packages/vite/src/node/plugins/importAnalysis.ts index f9fabbbfcdd298..31e9cd76faa8a0 100644 --- a/packages/vite/src/node/plugins/importAnalysis.ts +++ b/packages/vite/src/node/plugins/importAnalysis.ts @@ -112,7 +112,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { tryIndex: false, extensions: [] }) - let server: ViteDevServer | null = null + let server: ViteDevServer return { name: 'vite:import-analysis', @@ -122,9 +122,9 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { }, async transform(source, importer, options) { - // In a real app `server` is always defined, but it can be `null` when + // In a real app `server` is always defined, but it is undefined when // running src/node/server/__tests__/pluginContainer.spec.ts - if (server === null) { + if (!server) { return null } @@ -205,7 +205,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { let importerFile = importer if (moduleListContains(config.optimizeDeps?.exclude, url)) { - const optimizedDeps = server!._optimizedDeps + const optimizedDeps = server._optimizedDeps if (optimizedDeps) { await optimizedDeps.scanProcessing @@ -666,7 +666,7 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin { NULL_BYTE_PLACEHOLDER, '\0' ) - transformRequest(url, server!, { ssr }).catch((e) => { + transformRequest(url, server, { ssr }).catch((e) => { if (e?.code === ERR_OUTDATED_OPTIMIZED_DEP) { // This are expected errors return