From d0f9bc0ca37d15430678fc0cca0c7d823b314ba8 Mon Sep 17 00:00:00 2001 From: patrick Date: Wed, 11 Dec 2024 09:32:53 +0100 Subject: [PATCH] fix v2 addon watch --- .../addon-dev/src/rollup-app-reexports.ts | 11 ++- .../src/rollup-public-entrypoints.ts | 22 +++++- tests/scenarios/helpers/v2-addon.ts | 49 +++++++++++--- tests/scenarios/v2-addon-dev-watch-test.ts | 67 +++---------------- 4 files changed, 76 insertions(+), 73 deletions(-) diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 46e683041..f1e9cef17 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -3,6 +3,12 @@ import { extname } from 'path'; import minimatch from 'minimatch'; import type { Plugin } from 'rollup'; +function symmetricDifference(arr1: any[], arr2: any[]) { + return arr1 + .filter((x) => !arr2.includes(x)) + .concat(arr2.filter((x) => !arr1.includes(x))); +} + export default function appReexports(opts: { from: string; to: string; @@ -42,7 +48,10 @@ export default function appReexports(opts: { } let originalAppJS = pkg['ember-addon']?.['app-js']; - let hasChanges = JSON.stringify(originalAppJS) !== JSON.stringify(appJS); + let hasChanges = !!symmetricDifference( + Object.keys(originalAppJS), + Object.keys(appJS) + ).length; // Don't cause a file i/o event unless something actually changed if (hasChanges) { diff --git a/packages/addon-dev/src/rollup-public-entrypoints.ts b/packages/addon-dev/src/rollup-public-entrypoints.ts index 63bfa5df2..5695cc4be 100644 --- a/packages/addon-dev/src/rollup-public-entrypoints.ts +++ b/packages/addon-dev/src/rollup-public-entrypoints.ts @@ -13,17 +13,33 @@ export default function publicEntrypoints(args: { include: string[]; exclude?: string[]; }): Plugin { + const allEntryPoints: Set = new Set(); + return { name: 'addon-modules', async buildStart() { + const include = [ + ...args.include, + '**/*.hbs', + '**/*.ts', + '**/*.gts', + '**/*.gjs', + ]; + // wait a bit first https://github.com/nodejs/node/issues/4760 + await new Promise((resolve) => setTimeout(resolve, 50)); let matches = walkSync(args.srcDir, { - globs: [...args.include, '**/*.hbs', '**/*.ts', '**/*.gts', '**/*.gjs'], + globs: include, ignore: args.exclude, }); + matches.forEach((m) => allEntryPoints.add(m)); + + for (const name of allEntryPoints) { + this.addWatchFile(path.resolve(args.srcDir, name)); + } + for (let name of matches) { - this.addWatchFile(path.join(args.srcDir, name)); // the matched file, but with the extension swapped with .js let normalizedName = normalizeFileExt(name); @@ -58,7 +74,7 @@ export default function publicEntrypoints(args: { // additionally, we want to emit chunks where the pattern matches the supported // file extensions above (TS, GTS, etc) as if they were already the built JS. - let wouldMatchIfBuilt = args.include.some((pattern) => + let wouldMatchIfBuilt = include.some((pattern) => minimatch(normalizedName, pattern) ); diff --git a/tests/scenarios/helpers/v2-addon.ts b/tests/scenarios/helpers/v2-addon.ts index 95b5a5a38..09d3d69bc 100644 --- a/tests/scenarios/helpers/v2-addon.ts +++ b/tests/scenarios/helpers/v2-addon.ts @@ -2,9 +2,11 @@ import path from 'path'; import type { PreparedApp } from 'scenario-tester'; import { loadConfigFile } from 'rollup/loadConfigFile'; import rollup from 'rollup'; -import type { RollupOptions } from 'rollup'; +import type { RollupOptions, Plugin } from 'rollup'; export class DevWatcher { + #counter: number; + #expectedBuilds: number; #addon: PreparedApp; #watcher?: ReturnType<(typeof rollup)['watch']>; #waitForBuildPromise?: Promise; @@ -15,13 +17,17 @@ export class DevWatcher { constructor(addon: PreparedApp) { this.#addon = addon; this.#originalDirectory = process.cwd(); + this.#counter = 1; + this.#expectedBuilds = 1; } - start = async () => { + start = async (expectedBuilds = 1) => { if (this.#watcher) { throw new Error(`.start() may only be called once`); } + this.#expectedBuilds = expectedBuilds; + let buildDirectory = this.#addon.dir; /** @@ -39,6 +45,13 @@ export class DevWatcher { options.watch = { buildDelay: 20, }; + // for local debugging + (options.plugins as Plugin[]).push({ + name: 'watch change', + watchChange(id: string) { + console.log('changed:', id); + }, + }); return options; }) ); @@ -51,13 +64,18 @@ export class DevWatcher { this.#watcher.on('event', args => { switch (args.code) { case 'START': { - this.#defer(); + console.log('start'); break; } case 'END': { + console.log('end'); this.#forceResolve?.('end'); break; } + case 'BUNDLE_END': { + args.result.close(); + break; + } case 'ERROR': { this.#forceReject?.(args.error); break; @@ -67,29 +85,33 @@ export class DevWatcher { this.#watcher.on('close', () => this.#forceResolve?.('close')); - return this.settled(); + return this.nextBuild(); }; #defer = () => { if (this.#waitForBuildPromise) { // Need to finish prior work before deferring again // if we hit this use case, we may have mis-configured - // the previosu deferral + // the previous deferral return; } + this.#counter = this.#expectedBuilds; this.#waitForBuildPromise = new Promise((_resolve, _reject) => { - this.#resolve = _resolve; + this.#resolve = (...args) => { + this.#counter -= 1; + if (this.#counter === 0) { + _resolve(...args); + } + }; this.#reject = _reject; }); }; #forceResolve(state: unknown) { this.#resolve?.(state); - this.#waitForBuildPromise = undefined; } #forceReject(error: unknown) { this.#reject?.(error); - this.#waitForBuildPromise = undefined; } stop = async () => { @@ -97,12 +119,17 @@ export class DevWatcher { process.chdir(this.#originalDirectory); }; - nextBuild = async () => { + nextBuild = async (expectedBuilds = 1) => { + this.#expectedBuilds = expectedBuilds; this.#defer(); - await this.settled(); + try { + await this.settled(); + } finally { + this.#waitForBuildPromise = undefined; + } }; - settled = async (timeout = 5_000) => { + settled = async (timeout = 8_000) => { if (!this.#waitForBuildPromise) { console.debug(`There is nothing to wait for`); return; diff --git a/tests/scenarios/v2-addon-dev-watch-test.ts b/tests/scenarios/v2-addon-dev-watch-test.ts index ebc1b245d..a402e501c 100644 --- a/tests/scenarios/v2-addon-dev-watch-test.ts +++ b/tests/scenarios/v2-addon-dev-watch-test.ts @@ -184,7 +184,8 @@ Scenarios.fromProject(() => baseV2Addon()) ); await fs.rm(srcPathOther); - await watcher?.nextBuild(); + // we expect 2 builds because appReexports plugin modifies package.json triggering a new build + await watcher?.nextBuild(2); assert.strictEqual( existsSync(distAppReExportPathOther), false, @@ -194,7 +195,8 @@ Scenarios.fromProject(() => baseV2Addon()) test('create a component in src should create it in dist', async function (assert) { await fs.writeFile(srcPathOther, origContent); - await watcher?.nextBuild(); + // we expect 2 builds because appReexports plugin modifies package.json triggering a new build + await watcher?.nextBuild(2); assert.strictEqual( existsSync(distAppReExportPathOther), true, @@ -206,16 +208,9 @@ Scenarios.fromProject(() => baseV2Addon()) await becomesModified({ filePath: distPathDemoComp, assert, - // Update a component fn: async () => { let someContent = await fs.readFile(demoHbs); - - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast await fs.writeFile(demoHbs, someContent + `\n`); - await aBit(10); await watcher?.nextBuild(); }, }); @@ -225,36 +220,23 @@ Scenarios.fromProject(() => baseV2Addon()) await becomesModified({ filePath: distPathDemoComp, assert, - // Update a component fn: async () => { - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast - await aBit(10); await fs.rm(demoHbs); await watcher?.nextBuild(); }, }); }); - test('updating hbs content should not update unrelated files', async function (assert) { + test('creating hbs content should not update unrelated files', async function (assert) { await fs.writeFile(demoHbs, demoContent); await watcher?.nextBuild(); await isNotModified({ filePath: distPath, assert, - // Update a component fn: async () => { let someContent = await fs.readFile(demoHbs); - - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast await fs.writeFile(demoHbs, someContent + `\n\n`); - await aBit(10); await watcher?.nextBuild(); }, }); @@ -265,16 +247,9 @@ Scenarios.fromProject(() => baseV2Addon()) await isNotModified({ filePath: distPath, assert, - // Update a component fn: async () => { let someContent = await fs.readFile(demoHbs); - - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast await fs.writeFile(demoHbs, someContent + `\n`); - await aBit(10); await watcher?.nextBuild(); }, }); @@ -284,24 +259,15 @@ Scenarios.fromProject(() => baseV2Addon()) await becomesModified({ filePath: distPathOther, assert, - // Update a component fn: async () => { - await aBit(100); let someContent = await fs.readFile(srcPathOther); - - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast await fs.writeFile(srcPathOther, someContent + `test\n`); - await aBit(10); await watcher?.nextBuild(); }, }); }); test('deleting demo.js should make demo a template only component', async function (assert) { - await aBit(100); await fs.rm(demoJs); await watcher?.nextBuild(); let distPathDemoCompContent = await fs.readFile(distPathDemoComp); @@ -309,7 +275,6 @@ Scenarios.fromProject(() => baseV2Addon()) }); test('creating demo.js should make demo a template colocated component', async function (assert) { - await aBit(100); void fs.writeFile(demoJs, demoJsContent); await watcher?.nextBuild(); let distPathDemoCompContent = await fs.readFile(distPathDemoComp); @@ -332,11 +297,6 @@ Scenarios.fromProject(() => baseV2Addon()) fn: async () => { let someContent = await fs.readFile(someFile); - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast - await aBit(10); await fs.writeFile(someFile, someContent + `\n`); await watcher?.nextBuild(); }, @@ -354,16 +314,11 @@ Scenarios.fromProject(() => baseV2Addon()) assert, // Remove a component fn: async () => { - // generally it's bad to introduce time dependencies to a test, but we need to wait long enough - // to guess for how long it'll take for the file system to update our file. - // - // the `stat` is measured in `ms`, so it's still pretty fast - await aBit(10); - await Promise.all([ - fs.rm(path.join(addon.dir, 'src/components/demo.js')), - fs.rm(path.join(addon.dir, 'src/components/demo.hbs')), - ]); + await fs.rm(path.join(addon.dir, 'src/components/demo.js')); await watcher?.nextBuild(); + await fs.rm(path.join(addon.dir, 'src/components/demo.hbs')); + // we expect 2 builds because appReexports plugin modifies package.json triggering a new build + await watcher?.nextBuild(2); }, }); }); @@ -386,7 +341,3 @@ Scenarios.fromProject(() => baseV2Addon()) }); }); }); - -function aBit(ms: number) { - return new Promise(resolve => setTimeout(resolve, ms)); -}