From cc6ecd28a1155ed8ca877aa56bfe5302c832e484 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 12 May 2023 11:38:16 -0400 Subject: [PATCH 1/8] Resolve https://github.com/embroider-build/embroider/issues/1420 --- .../addon-dev/src/rollup-app-reexports.ts | 79 ++++++++++++++++++- 1 file changed, 75 insertions(+), 4 deletions(-) diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 9e72c3809..70b756a34 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -32,10 +32,81 @@ export default function appReexports(opts: { }); } } - pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { - 'app-js': appJS, - }); - writeJsonSync('package.json', pkg, { spaces: 2 }); + + // Don't cause a file i/o event unless something actually changed + if (hasChanges(pkg?.['ember-addon']?.['app-js'], appJS)) { + pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { + 'app-js': appJS, + }); + writeJsonSync('package.json', pkg, { spaces: 2 }); + } }, }; } + +/** + * Not "deep equality", but "not quite shallow, value-based equality" + * and somewhat narrowly scoped to what we care about in the "app-js" + * structure (which is 'just JSON') + * + * The algo here is: + * - short-circuit as early as possible to do as little work as possible + * - check properties of objects before checking contents, + * because checking contents is more expensive + * - relies on the fact that we can treat arrays and objects the same due to + * how JavaScript works + */ +function hasChanges(a: undefined | unknown, b: undefined | unknown) { + if (a === b) return false; + if (typeof a !== typeof b) return true; + + // From this point on, a and b have the same type + // and are truthy values + switch (typeof a) { + case 'string': + return a !== b; + case 'number': + return a !== b; + case 'object': + // TS does not narrow here, + // given b *is* known at this point, + // due to `typeof a !== typeof b` has an early return. + // + // TS does not not maintain the relationship of typeof a === typeof b + if (typeof b !== 'object') return true; + + return doesObjectHaveChanges(a, b); + default: + // for undefined, function, symbol, bigint (none of these are valid JSON values) + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof + return true; + } +} + +// Type `object` differs from `typeof` object +function doesObjectHaveChanges(a: object | null, b: object | null) { + // Need to ensure the values are turthy so that we can use Object.entries + // This is because 'null' is an object + if (!a || !b) return true; + + // Both Arrays and Objects behave the same when passed to Object.entries + let aEntries = Object.entries(a); + let bEntries = Object.entries(b); + + // Differnt sets of entries means there are changes + if (aEntries.length !== bEntries.length) return true; + + // With aEntries and bEntries being the same length, we want to check if + // each entry within each matches the entry within the other. + // To do this efficiently, we need to use a "more traditional" for loop. + for (let i = 0; i < aEntries.length; i++) { + let aEntry = aEntries[i]; + let bEntry = bEntries[i]; + + // The key + if (aEntry[0] !== bEntry[0]) return true; + + // The value + if (hasChanges(aEntry[1], bEntry[1])) return true; + } +} From 921fc4498547b806137cfdc39c0f5acaaeabf8cd Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 12 May 2023 11:40:37 -0400 Subject: [PATCH 2/8] Use consistent returns and add explicit return type --- packages/addon-dev/src/rollup-app-reexports.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 70b756a34..16ac4ae2d 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -56,7 +56,7 @@ export default function appReexports(opts: { * - relies on the fact that we can treat arrays and objects the same due to * how JavaScript works */ -function hasChanges(a: undefined | unknown, b: undefined | unknown) { +function hasChanges(a: undefined | unknown, b: undefined | unknown): boolean { if (a === b) return false; if (typeof a !== typeof b) return true; @@ -84,7 +84,7 @@ function hasChanges(a: undefined | unknown, b: undefined | unknown) { } // Type `object` differs from `typeof` object -function doesObjectHaveChanges(a: object | null, b: object | null) { +function doesObjectHaveChanges(a: object | null, b: object | null): boolean { // Need to ensure the values are turthy so that we can use Object.entries // This is because 'null' is an object if (!a || !b) return true; @@ -109,4 +109,6 @@ function doesObjectHaveChanges(a: object | null, b: object | null) { // The value if (hasChanges(aEntry[1], bEntry[1])) return true; } + + return false; } From 571be7148312361f403bf6846922596620e49c18 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 12 May 2023 12:00:21 -0400 Subject: [PATCH 3/8] Move new functions to their own file --- .../addon-dev/src/rollup-app-reexports.ts | 70 +----------------- packages/addon-dev/src/utils.ts | 72 +++++++++++++++++++ 2 files changed, 73 insertions(+), 69 deletions(-) create mode 100644 packages/addon-dev/src/utils.ts diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 16ac4ae2d..0d2fafcf5 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -1,6 +1,7 @@ import { readJsonSync, writeJsonSync } from 'fs-extra'; import { extname } from 'path'; import minimatch from 'minimatch'; +import { hasChanges } from './utils'; import type { Plugin } from 'rollup'; export default function appReexports(opts: { @@ -43,72 +44,3 @@ export default function appReexports(opts: { }, }; } - -/** - * Not "deep equality", but "not quite shallow, value-based equality" - * and somewhat narrowly scoped to what we care about in the "app-js" - * structure (which is 'just JSON') - * - * The algo here is: - * - short-circuit as early as possible to do as little work as possible - * - check properties of objects before checking contents, - * because checking contents is more expensive - * - relies on the fact that we can treat arrays and objects the same due to - * how JavaScript works - */ -function hasChanges(a: undefined | unknown, b: undefined | unknown): boolean { - if (a === b) return false; - if (typeof a !== typeof b) return true; - - // From this point on, a and b have the same type - // and are truthy values - switch (typeof a) { - case 'string': - return a !== b; - case 'number': - return a !== b; - case 'object': - // TS does not narrow here, - // given b *is* known at this point, - // due to `typeof a !== typeof b` has an early return. - // - // TS does not not maintain the relationship of typeof a === typeof b - if (typeof b !== 'object') return true; - - return doesObjectHaveChanges(a, b); - default: - // for undefined, function, symbol, bigint (none of these are valid JSON values) - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof - return true; - } -} - -// Type `object` differs from `typeof` object -function doesObjectHaveChanges(a: object | null, b: object | null): boolean { - // Need to ensure the values are turthy so that we can use Object.entries - // This is because 'null' is an object - if (!a || !b) return true; - - // Both Arrays and Objects behave the same when passed to Object.entries - let aEntries = Object.entries(a); - let bEntries = Object.entries(b); - - // Differnt sets of entries means there are changes - if (aEntries.length !== bEntries.length) return true; - - // With aEntries and bEntries being the same length, we want to check if - // each entry within each matches the entry within the other. - // To do this efficiently, we need to use a "more traditional" for loop. - for (let i = 0; i < aEntries.length; i++) { - let aEntry = aEntries[i]; - let bEntry = bEntries[i]; - - // The key - if (aEntry[0] !== bEntry[0]) return true; - - // The value - if (hasChanges(aEntry[1], bEntry[1])) return true; - } - - return false; -} diff --git a/packages/addon-dev/src/utils.ts b/packages/addon-dev/src/utils.ts new file mode 100644 index 000000000..6afc3e0fd --- /dev/null +++ b/packages/addon-dev/src/utils.ts @@ -0,0 +1,72 @@ +/** + * Not "deep equality", but "not quite shallow, value-based equality" + * and somewhat narrowly scoped to what we care about in the "app-js" + * structure (which is 'just JSON') (and other objects within the 'ember-addon' + * data that the addon-dev family of plugins manage) + * + * The algo here is: + * - short-circuit as early as possible to do as little work as possible + * - check properties of objects before checking contents, + * because checking contents is more expensive + * - relies on the fact that we can treat arrays and objects the same due to + * how JavaScript works + */ +export function hasChanges( + a: undefined | unknown, + b: undefined | unknown +): boolean { + if (a === b) return false; + if (typeof a !== typeof b) return true; + + // From this point on, a and b have the same type + // and are truthy values + switch (typeof a) { + case 'string': + return a !== b; + case 'number': + return a !== b; + case 'object': + // TS does not narrow here, + // given b *is* known at this point, + // due to `typeof a !== typeof b` has an early return. + // + // TS does not not maintain the relationship of typeof a === typeof b + if (typeof b !== 'object') return true; + + return doesObjectHaveChanges(a, b); + default: + // for undefined, function, symbol, bigint (none of these are valid JSON values) + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof + return true; + } +} + +// Type `object` differs from `typeof` object +function doesObjectHaveChanges(a: object | null, b: object | null): boolean { + // Need to ensure the values are turthy so that we can use Object.entries + // This is because 'null' is an object + if (!a || !b) return true; + + // Both Arrays and Objects behave the same when passed to Object.entries + let aEntries = Object.entries(a); + let bEntries = Object.entries(b); + + // Differnt sets of entries means there are changes + if (aEntries.length !== bEntries.length) return true; + + // With aEntries and bEntries being the same length, we want to check if + // each entry within each matches the entry within the other. + // To do this efficiently, we need to use a "more traditional" for loop. + for (let i = 0; i < aEntries.length; i++) { + let aEntry = aEntries[i]; + let bEntry = bEntries[i]; + + // The key + if (aEntry[0] !== bEntry[0]) return true; + + // The value + if (hasChanges(aEntry[1], bEntry[1])) return true; + } + + return false; +} From 6de3528ef6a8e6e058016acf7c9edbb2225ff0ac Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Fri, 12 May 2023 12:23:30 -0400 Subject: [PATCH 4/8] Public assets needed the same condition --- packages/addon-dev/src/rollup-app-reexports.ts | 2 +- packages/addon-dev/src/rollup-public-assets.ts | 11 +++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 0d2fafcf5..14b4fb886 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -35,7 +35,7 @@ export default function appReexports(opts: { } // Don't cause a file i/o event unless something actually changed - if (hasChanges(pkg?.['ember-addon']?.['app-js'], appJS)) { + if (hasChanges(pkg['ember-addon']?.['app-js'], appJS)) { pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { 'app-js': appJS, }); diff --git a/packages/addon-dev/src/rollup-public-assets.ts b/packages/addon-dev/src/rollup-public-assets.ts index 6856624d1..cbb82af8a 100644 --- a/packages/addon-dev/src/rollup-public-assets.ts +++ b/packages/addon-dev/src/rollup-public-assets.ts @@ -1,6 +1,7 @@ import { readJsonSync, writeJsonSync } from 'fs-extra'; import walkSync from 'walk-sync'; import type { Plugin } from 'rollup'; +import { hasChanges } from './utils'; export default function publicAssets( path: string, @@ -26,11 +27,13 @@ export default function publicAssets( {} ); - pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { - 'public-assets': publicAssets, - }); + if (hasChanges(pkg['ember-addon']?.['public-assets'], publicAssets)) { + pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { + 'public-assets': publicAssets, + }); - writeJsonSync('package.json', pkg, { spaces: 2 }); + writeJsonSync('package.json', pkg, { spaces: 2 }); + } }, }; } From c41af5f7ba53e580123e25db6c356173f4d526a7 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 16 May 2023 11:43:33 -0400 Subject: [PATCH 5/8] Simplify hasChanges logic via JSON.stringify --- .../addon-dev/src/rollup-app-reexports.ts | 7 +- .../addon-dev/src/rollup-public-assets.ts | 8 ++- packages/addon-dev/src/utils.ts | 72 ------------------- 3 files changed, 11 insertions(+), 76 deletions(-) delete mode 100644 packages/addon-dev/src/utils.ts diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index 14b4fb886..fdf969492 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -1,7 +1,6 @@ import { readJsonSync, writeJsonSync } from 'fs-extra'; import { extname } from 'path'; import minimatch from 'minimatch'; -import { hasChanges } from './utils'; import type { Plugin } from 'rollup'; export default function appReexports(opts: { @@ -33,9 +32,13 @@ export default function appReexports(opts: { }); } } + let originalPublicAssets = pkg['ember-addon']?.['app-js']; + + let hasChanges = + JSON.stringify(originalPublicAssets) !== JSON.stringify(appJS); // Don't cause a file i/o event unless something actually changed - if (hasChanges(pkg['ember-addon']?.['app-js'], appJS)) { + if (hasChanges) { pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { 'app-js': appJS, }); diff --git a/packages/addon-dev/src/rollup-public-assets.ts b/packages/addon-dev/src/rollup-public-assets.ts index cbb82af8a..80147ead8 100644 --- a/packages/addon-dev/src/rollup-public-assets.ts +++ b/packages/addon-dev/src/rollup-public-assets.ts @@ -1,7 +1,6 @@ import { readJsonSync, writeJsonSync } from 'fs-extra'; import walkSync from 'walk-sync'; import type { Plugin } from 'rollup'; -import { hasChanges } from './utils'; export default function publicAssets( path: string, @@ -27,7 +26,12 @@ export default function publicAssets( {} ); - if (hasChanges(pkg['ember-addon']?.['public-assets'], publicAssets)) { + let originalPublicAssets = pkg['ember-addon']?.['public-assets']; + + let hasChanges = + JSON.stringify(originalPublicAssets) !== JSON.stringify(publicAssets); + + if (hasChanges) { pkg['ember-addon'] = Object.assign({}, pkg['ember-addon'], { 'public-assets': publicAssets, }); diff --git a/packages/addon-dev/src/utils.ts b/packages/addon-dev/src/utils.ts deleted file mode 100644 index 6afc3e0fd..000000000 --- a/packages/addon-dev/src/utils.ts +++ /dev/null @@ -1,72 +0,0 @@ -/** - * Not "deep equality", but "not quite shallow, value-based equality" - * and somewhat narrowly scoped to what we care about in the "app-js" - * structure (which is 'just JSON') (and other objects within the 'ember-addon' - * data that the addon-dev family of plugins manage) - * - * The algo here is: - * - short-circuit as early as possible to do as little work as possible - * - check properties of objects before checking contents, - * because checking contents is more expensive - * - relies on the fact that we can treat arrays and objects the same due to - * how JavaScript works - */ -export function hasChanges( - a: undefined | unknown, - b: undefined | unknown -): boolean { - if (a === b) return false; - if (typeof a !== typeof b) return true; - - // From this point on, a and b have the same type - // and are truthy values - switch (typeof a) { - case 'string': - return a !== b; - case 'number': - return a !== b; - case 'object': - // TS does not narrow here, - // given b *is* known at this point, - // due to `typeof a !== typeof b` has an early return. - // - // TS does not not maintain the relationship of typeof a === typeof b - if (typeof b !== 'object') return true; - - return doesObjectHaveChanges(a, b); - default: - // for undefined, function, symbol, bigint (none of these are valid JSON values) - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/typeof - return true; - } -} - -// Type `object` differs from `typeof` object -function doesObjectHaveChanges(a: object | null, b: object | null): boolean { - // Need to ensure the values are turthy so that we can use Object.entries - // This is because 'null' is an object - if (!a || !b) return true; - - // Both Arrays and Objects behave the same when passed to Object.entries - let aEntries = Object.entries(a); - let bEntries = Object.entries(b); - - // Differnt sets of entries means there are changes - if (aEntries.length !== bEntries.length) return true; - - // With aEntries and bEntries being the same length, we want to check if - // each entry within each matches the entry within the other. - // To do this efficiently, we need to use a "more traditional" for loop. - for (let i = 0; i < aEntries.length; i++) { - let aEntry = aEntries[i]; - let bEntry = bEntries[i]; - - // The key - if (aEntry[0] !== bEntry[0]) return true; - - // The value - if (hasChanges(aEntry[1], bEntry[1])) return true; - } - - return false; -} From 7d18d2017ad5d646387754558bb0d2e3e15cc48b Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Tue, 16 May 2023 11:44:28 -0400 Subject: [PATCH 6/8] Fix variable name --- packages/addon-dev/src/rollup-app-reexports.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/addon-dev/src/rollup-app-reexports.ts b/packages/addon-dev/src/rollup-app-reexports.ts index fdf969492..036852367 100644 --- a/packages/addon-dev/src/rollup-app-reexports.ts +++ b/packages/addon-dev/src/rollup-app-reexports.ts @@ -32,10 +32,9 @@ export default function appReexports(opts: { }); } } - let originalPublicAssets = pkg['ember-addon']?.['app-js']; + let originalAppJS = pkg['ember-addon']?.['app-js']; - let hasChanges = - JSON.stringify(originalPublicAssets) !== JSON.stringify(appJS); + let hasChanges = JSON.stringify(originalAppJS) !== JSON.stringify(appJS); // Don't cause a file i/o event unless something actually changed if (hasChanges) { From 1ef57d6829abce51fdb33545e2969cafd240f072 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 18 May 2023 15:48:18 -0400 Subject: [PATCH 7/8] Add a test for testing the watch mode behavior and expectations of our rollup plugins --- tests/scenarios/v2-addon-dev-watch-test.ts | 329 +++++++++++++++++++++ 1 file changed, 329 insertions(+) create mode 100644 tests/scenarios/v2-addon-dev-watch-test.ts diff --git a/tests/scenarios/v2-addon-dev-watch-test.ts b/tests/scenarios/v2-addon-dev-watch-test.ts new file mode 100644 index 000000000..d5d347e28 --- /dev/null +++ b/tests/scenarios/v2-addon-dev-watch-test.ts @@ -0,0 +1,329 @@ +import path from 'path'; +import { baseV2Addon } from './scenarios'; +import { PreparedApp, Scenarios } from 'scenario-tester'; +import fs from 'fs/promises'; +import { spawn } from 'child_process'; +import QUnit from 'qunit'; +import merge from 'lodash/merge'; + +const { module: Qmodule, test } = QUnit; + +Scenarios.fromProject(() => baseV2Addon()) + .map('v2-addon-dev-watch', async addon => { + addon.pkg.name = 'v2-addon'; + addon.pkg.files = ['dist']; + addon.pkg.exports = { + './*': './dist/*.js', + './addon-main.js': './addon-main.js', + './package.json': './package.json', + }; + addon.pkg.scripts = { + build: 'node ./node_modules/rollup/dist/bin/rollup -c ./rollup.config.mjs', + start: 'node ./node_modules/rollup/dist/bin/rollup -c ./rollup.config.mjs --watch --no-watch.clearScreen', + }; + + merge(addon.files, { + 'babel.config.json': ` + { + "presets": [ + ["@babel/preset-env"] + ], + "plugins": [ + "@embroider/addon-dev/template-colocation-plugin", + ["@babel/plugin-proposal-decorators", { "legacy": true }], + [ "@babel/plugin-proposal-class-properties" ] + ] + } + `, + 'rollup.config.mjs': ` + import { babel } from '@rollup/plugin-babel'; + import { Addon } from '@embroider/addon-dev/rollup'; + + const addon = new Addon({ + srcDir: 'src', + destDir: 'dist', + }); + + export default { + output: addon.output(), + + plugins: [ + addon.publicEntrypoints(['components/**/*.js']), + addon.appReexports(['components/**/*.js']), + addon.hbs(), + addon.dependencies(), + addon.publicAssets('custom-public'), + + babel({ babelHelpers: 'bundled' }), + + addon.clean(), + ], + }; + `, + 'custom-public': { + 'demo.css': `button { color: red; }`, + 'index.css': `button { color: green; }`, + }, + src: { + components: { + 'button.hbs': ` + + `, + 'out.hbs': `{{yield}}`, + 'demo.js': ` + import Component from '@glimmer/component'; + import { tracked } from '@glimmer/tracking'; + + import FlipButton from './button'; + import Out from './out'; + + export default class ExampleComponent extends Component { + Button = FlipButton; + Out = Out; + + @tracked active = false; + + flip = () => (this.active = !this.active); + } + `, + 'demo.hbs': `Hello there! + {{this.active}} + + + `, + }, + }, + }); + + addon.linkDependency('@embroider/addon-shim', { baseDir: __dirname }); + addon.linkDependency('@embroider/addon-dev', { baseDir: __dirname }); + addon.linkDependency('babel-plugin-ember-template-compilation', { baseDir: __dirname }); + addon.linkDevDependency('@babel/core', { baseDir: __dirname }); + addon.linkDevDependency('@babel/plugin-proposal-class-properties', { baseDir: __dirname }); + addon.linkDevDependency('@babel/plugin-proposal-decorators', { baseDir: __dirname }); + addon.linkDevDependency('@babel/preset-env', { baseDir: __dirname }); + addon.linkDevDependency('@rollup/plugin-babel', { baseDir: __dirname }); + addon.linkDevDependency('rollup', { baseDir: __dirname }); + }) + .forEachScenario(scenario => { + Qmodule(scenario.name, function (hooks) { + let addon: PreparedApp; + let watcher: DevWatcher | undefined; + + hooks.before(async () => { + addon = await scenario.prepare(); + + // run the build *once* so we have a known stable state + await addon.execute('pnpm build'); + }); + + hooks.beforeEach(function (assert) { + // None of these tests should take longer than even 1s, but + // if something goes wrong, they could hang, and we don't want to hold up + // all of C.I. + assert.timeout(5_000); + }); + + hooks.afterEach(async () => { + watcher?.stop(); + }); + + Qmodule('Watching the addon via rollup -c -w', function () { + test('the package.json is not updated since it would be the same', async function (assert) { + watcher = new DevWatcher(addon); + + await watcher.start(); + + let someFile = path.join(addon.dir, 'src/components/demo.hbs'); + let manifestPath = path.join(addon.dir, 'package.json'); + + await isNotModified({ + filePath: manifestPath, + assert, + // Update a component + 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`); + }, + }); + }); + + test('the package.json *is* updated, since app-js changed', async function (assert) { + watcher = new DevWatcher(addon); + + await watcher.start(); + + let manifestPath = path.join(addon.dir, 'package.json'); + + await becomesModified({ + filePath: manifestPath, + 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 fs.rm(path.join(addon.dir, 'src/components/demo.js')); + await fs.rm(path.join(addon.dir, 'src/components/demo.hbs')); + + await watcher?.settled(); + }, + }); + }); + + test('the package.json *is* updated, since public assets changed', async function (assert) { + let someFile = path.join(addon.dir, 'custom-public/demo.css'); + let manifestPath = path.join(addon.dir, 'package.json'); + + await becomesModified({ + filePath: manifestPath, + assert, + // Delete a publicAsset + fn: async () => { + await fs.rm(someFile); + // publicAssets are not watched, as they are not part of The Module Graph™ + await addon.execute('pnpm build'); + }, + }); + }); + }); + }); + }); + +class DevWatcher { + #addon: PreparedApp; + #singletonAbort?: AbortController; + #waitForBuildPromise?: Promise; + #lastBuild?: string; + + constructor(addon: PreparedApp) { + this.#addon = addon; + } + + start = () => { + if (this.#singletonAbort) this.#singletonAbort.abort(); + + this.#singletonAbort = new AbortController(); + + /** + * NOTE: when running rollup in a non-TTY environemnt, the "watching for changes" message does not print. + */ + let rollupProcess = spawn('pnpm', ['start'], { + cwd: this.#addon.dir, + signal: this.#singletonAbort.signal, + stdio: ['pipe', 'pipe', 'pipe'], + // Have to disable color so our regex / string matching works easier + // Have to include process.env, so the spawned environment has access to `pnpm` + env: { ...process.env, NO_COLOR: '1' }, + }); + + let settle: (...args: unknown[]) => void; + let error: (...args: unknown[]) => void; + this.#waitForBuildPromise = new Promise((resolve, reject) => { + settle = resolve; + error = reject; + }); + + if (!rollupProcess.stdout) { + throw new Error(`Failed to start process, pnpm start`); + } + if (!rollupProcess.stderr) { + throw new Error(`Failed to start process, pnpm start`); + } + + let handleData = (data: Buffer) => { + let string = data.toString(); + let lines = string.split('\n'); + + let build = lines.find(line => line.trim().match(/^created dist in (.+)$/)); + let problem = lines.find(line => line.includes('Error:')); + let isAbort = lines.find(line => line.includes('AbortError:')); + + if (isAbort) { + // Test may have ended, we want to kill the watcher, + // but not error, because throwing an error causes the test to fail. + return settle(); + } + + if (problem) { + console.error('\n!!!\n', problem, '\n!!!\n'); + error(problem); + return; + } + + if (build) { + this.#lastBuild = build[1]; + + settle?.(); + + this.#waitForBuildPromise = new Promise((resolve, reject) => { + settle = resolve; + error = reject; + }); + } + }; + + // NOTE: rollup outputs to stderr only, not stdout + rollupProcess.stderr.on('data', (...args) => handleData(...args)); + rollupProcess.on('error', handleData); + rollupProcess.on('close', () => settle?.()); + rollupProcess.on('exit', () => settle?.()); + + return this.#waitForBuildPromise; + }; + + stop = () => this.#singletonAbort?.abort(); + settled = () => this.#waitForBuildPromise; + get lastBuild() { + return this.#lastBuild; + } +} +async function becomesModified({ + filePath, + assert, + fn, +}: { + filePath: string; + assert: Assert; + fn: () => Promise; +}) { + let oldStat = (await fs.stat(filePath)).mtimeMs; + + await fn(); + + let newStat = (await fs.stat(filePath)).mtimeMs; + + assert.notStrictEqual( + oldStat, + newStat, + `Expected ${filePath} to be modified. Latest: ${newStat}, previously: ${oldStat}` + ); +} + +async function isNotModified({ filePath, assert, fn }: { filePath: string; assert: Assert; fn: () => Promise }) { + let oldStat = (await fs.stat(filePath)).mtimeMs; + + await fn(); + + let newStat = (await fs.stat(filePath)).mtimeMs; + + assert.strictEqual( + oldStat, + newStat, + `Expected ${filePath} to be unchanged. Latest: ${newStat}, and pre-fn: ${oldStat}` + ); +} + +function aBit(ms: number) { + return new Promise(resolve => setTimeout(resolve, ms)); +} From 26a768ff1b06981b4425256f9ea54e6d216ed6d7 Mon Sep 17 00:00:00 2001 From: NullVoxPopuli <199018+NullVoxPopuli@users.noreply.github.com> Date: Thu, 18 May 2023 19:30:40 -0400 Subject: [PATCH 8/8] Move non-watching test outside the watching Q-module --- tests/scenarios/v2-addon-dev-watch-test.ts | 28 +++++++++++----------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/tests/scenarios/v2-addon-dev-watch-test.ts b/tests/scenarios/v2-addon-dev-watch-test.ts index d5d347e28..70ae90872 100644 --- a/tests/scenarios/v2-addon-dev-watch-test.ts +++ b/tests/scenarios/v2-addon-dev-watch-test.ts @@ -180,21 +180,21 @@ Scenarios.fromProject(() => baseV2Addon()) }, }); }); + }); - test('the package.json *is* updated, since public assets changed', async function (assert) { - let someFile = path.join(addon.dir, 'custom-public/demo.css'); - let manifestPath = path.join(addon.dir, 'package.json'); - - await becomesModified({ - filePath: manifestPath, - assert, - // Delete a publicAsset - fn: async () => { - await fs.rm(someFile); - // publicAssets are not watched, as they are not part of The Module Graph™ - await addon.execute('pnpm build'); - }, - }); + test('the package.json *is* updated on a rebuild, since public assets changed', async function (assert) { + let someFile = path.join(addon.dir, 'custom-public/demo.css'); + let manifestPath = path.join(addon.dir, 'package.json'); + + await becomesModified({ + filePath: manifestPath, + assert, + // Delete a publicAsset + fn: async () => { + await fs.rm(someFile); + // publicAssets are not watched, as they are not part of The Module Graph™ + await addon.execute('pnpm build'); + }, }); }); });