From 7acbb1a9998edc060259d54582149a2596159aed Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Fri, 10 Jul 2020 09:36:55 -0400 Subject: [PATCH 01/11] Improve logging for manifest builder --- packages/server/src/manifest-builder.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index d48f11878..5621838c7 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -14,8 +14,6 @@ import { Test } from '@bigtest/suite'; import { OrchestratorState } from './orchestrator/state'; -const { copyFile, mkdir, truncate } = fs.promises; - interface ManifestBuilderOptions { delegate: Mailbox; atom: Atom; @@ -30,7 +28,7 @@ export function* updateSourceMapURL(filePath: string, sourcemapName: string): Op let [currentURL]: [Buffer] = yield once(readStream, 'data'); if (currentURL.toString().trim() === 'manifest.js.map') { - yield truncate(filePath, size - 16); + fs.truncateSync(filePath, size - 16); fs.appendFileSync(filePath, sourcemapName); } else { throw new Error(`Expected a sourcemapping near the end of the generated test bundle, but found "${currentURL}" instead.`); @@ -46,9 +44,9 @@ function* processManifest(options: ManifestBuilderOptions): Operation { let distPath = path.resolve(options.distDir, fileName); let mapPath = path.resolve(options.distDir, sourcemapName); - yield mkdir(path.dirname(distPath), { recursive: true }); - yield copyFile(buildPath, distPath); - yield copyFile(sourcemapDir, mapPath); + fs.mkdirSync(path.dirname(distPath), { recursive: true }); + fs.copyFileSync(buildPath, distPath); + fs.copyFileSync(sourcemapDir, mapPath); yield updateSourceMapURL(distPath, sourcemapName); let manifest = yield import(distPath); @@ -66,7 +64,9 @@ function* processManifest(options: ManifestBuilderOptions): Operation { function logBuildError(error: BundlerError) { console.error("[manifest builder] build error:", error.message); - console.error("[manifest builder] build error frame:\n", error.frame); + if (error.frame) { + console.error("[manifest builder] build error frame:\n", error.frame); + } } function* waitForSuccessfulBuild(bundlerEvents: ChainableSubscription, delegate: Mailbox): Operation { @@ -103,7 +103,7 @@ export function* createManifestBuilder(options: ManifestBuilderOptions): Operati options.delegate.send({ event: "error" }); } else { let distPath = yield processManifest(options); - console.debug("[manifest builder] manifest updated"); + console.info("[manifest builder] manifest updated"); options.delegate.send({ event: "update", path: distPath }); } }); From e1d325db66014429c8d3a3374f0b3d4d1361d97d Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Fri, 10 Jul 2020 09:37:41 -0400 Subject: [PATCH 02/11] Put more inside `try` block in bundler --- packages/bundler/src/bundler.ts | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index 7e4fb7f52..370c3c26d 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -2,7 +2,7 @@ import { Operation, resource } from 'effection'; import { on } from '@effection/events'; import { Subscribable, SymbolSubscribable } from '@effection/subscription'; import { Channel } from '@effection/channel'; -import { watch, RollupWatchOptions, RollupWatcherEvent } from 'rollup'; +import { watch, RollupWatchOptions, RollupWatcherEvent, RollupWatcher } from 'rollup'; import resolve from '@rollup/plugin-node-resolve'; import * as commonjs from '@rollup/plugin-commonjs'; // eslint-disable-next-line @typescript-eslint/ban-ts-ignore @@ -64,20 +64,24 @@ export class Bundler implements Subscribable { static *create(bundles: Array): Operation { let bundler = new Bundler(); - let rollup = watch(prepareRollupOptions(bundles)); - let events = Subscribable - .from(on>(rollup, 'event')) - .map(([event]) => event) - .filter(event => event.code === 'END' || event.code === 'ERROR') - .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); return yield resource(bundler, function*() { + let rollup: RollupWatcher | null = null; + try { - yield events.forEach(function*(message) { - bundler.channel.send(message as BundlerMessage); - }); + rollup = watch(prepareRollupOptions(bundles)); + let events = Subscribable + .from(on>(rollup, 'event')) + .map(([event]) => event) + .filter(event => event.code === 'END' || event.code === 'ERROR') + .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); + yield events.forEach(function*(message) { + bundler.channel.send(message as BundlerMessage); + }); } finally { - rollup.close(); + if (rollup) { + rollup.close(); + } } }); } From fa069b21bb098ce32403867764ddeafd47aa00bc Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Fri, 10 Jul 2020 11:33:40 -0400 Subject: [PATCH 03/11] Properly ignore node_modules from manifest build watcher --- packages/bundler/src/bundler.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index 370c3c26d..891f2cdca 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -28,6 +28,9 @@ export type BundlerMessage = | { type: 'error'; error: BundlerError }; function prepareRollupOptions(bundles: Array, { mainFields }: BundlerOptions = { mainFields: ["browser", "main"] }): Array { + // Rollup types are wrong; `watch.exclude` allows RegExp[] + // eslint-disable-next-line @typescript-eslint/ban-ts-ignore + // @ts-ignore return bundles.map(bundle => { return { input: bundle.entry, @@ -38,7 +41,7 @@ function prepareRollupOptions(bundles: Array, { mainFields }: Bun format: 'umd', }, watch: { - exclude: ['node_modules/**'] + exclude: [/node_modules/] }, plugins: [ resolve({ From 3df027c8e24508424eb9295d61750992a0848f26 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Sat, 11 Jul 2020 22:00:07 -0400 Subject: [PATCH 04/11] Refactor Subscribable.from() -> subscribe() in Bundler --- packages/bundler/src/bundler.ts | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index 891f2cdca..ebfc36c37 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -1,6 +1,6 @@ import { Operation, resource } from 'effection'; import { on } from '@effection/events'; -import { Subscribable, SymbolSubscribable } from '@effection/subscription'; +import { subscribe, Subscribable, SymbolSubscribable, ChainableSubscription } from '@effection/subscription'; import { Channel } from '@effection/channel'; import { watch, RollupWatchOptions, RollupWatcherEvent, RollupWatcher } from 'rollup'; import resolve from '@rollup/plugin-node-resolve'; @@ -73,14 +73,15 @@ export class Bundler implements Subscribable { try { rollup = watch(prepareRollupOptions(bundles)); - let events = Subscribable - .from(on>(rollup, 'event')) + let events: ChainableSubscription, undefined> = yield subscribe(on>(rollup, 'event')); + let messages = events .map(([event]) => event) .filter(event => event.code === 'END' || event.code === 'ERROR') .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); - yield events.forEach(function*(message) { - bundler.channel.send(message as BundlerMessage); - }); + + yield messages.forEach(function*(message) { + bundler.channel.send(message as BundlerMessage); + }); } finally { if (rollup) { rollup.close(); From b116f9ccf733380c09f3ab9cc1132d4b80c3a096 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Sun, 12 Jul 2020 13:04:18 -0400 Subject: [PATCH 05/11] Fix fs.promises.truncate() bug more precisely --- packages/server/src/manifest-builder.ts | 35 ++++++++++++++++++++----- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index 5621838c7..7c9f0eea5 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -2,7 +2,7 @@ import { bigtestGlobals } from '@bigtest/globals'; import { Operation } from 'effection'; import { once } from '@effection/events'; import { subscribe, ChainableSubscription } from '@effection/subscription'; -import { Mailbox } from '@bigtest/effection'; +import { Mailbox, ensure } from '@bigtest/effection'; import { Bundler, BundlerMessage, BundlerError } from '@bigtest/bundler'; import { Atom } from '@bigtest/atom'; import { createFingerprint } from 'fprint'; @@ -14,6 +14,8 @@ import { Test } from '@bigtest/suite'; import { OrchestratorState } from './orchestrator/state'; +const { copyFile, mkdir, stat, appendFile, open } = fs.promises; + interface ManifestBuilderOptions { delegate: Mailbox; atom: Atom; @@ -22,14 +24,33 @@ interface ManifestBuilderOptions { distDir: string; }; +function ftruncate(fd: number, len: number): Operation { + return ({ fail, resume }) => { + fs.ftruncate(fd, len, err => { + if (err) { + fail(err); + return; + } + resume(); + }) + }; +} + +// https://github.com/nodejs/node/issues/34189#issuecomment-654878715 +function* truncate(path: string, len: number): Operation { + let file: fs.promises.FileHandle = yield open(path, 'r+'); + yield ensure(() => file.close()); + yield ftruncate(file.fd, len); +} + export function* updateSourceMapURL(filePath: string, sourcemapName: string): Operation{ - let { size } = fs.statSync(filePath); + let { size } = yield stat(filePath); let readStream = fs.createReadStream(filePath, {start: size - 16}); let [currentURL]: [Buffer] = yield once(readStream, 'data'); if (currentURL.toString().trim() === 'manifest.js.map') { - fs.truncateSync(filePath, size - 16); - fs.appendFileSync(filePath, sourcemapName); + yield truncate(filePath, size - 16); + yield appendFile(filePath, sourcemapName); } else { throw new Error(`Expected a sourcemapping near the end of the generated test bundle, but found "${currentURL}" instead.`); }; @@ -44,9 +65,9 @@ function* processManifest(options: ManifestBuilderOptions): Operation { let distPath = path.resolve(options.distDir, fileName); let mapPath = path.resolve(options.distDir, sourcemapName); - fs.mkdirSync(path.dirname(distPath), { recursive: true }); - fs.copyFileSync(buildPath, distPath); - fs.copyFileSync(sourcemapDir, mapPath); + yield mkdir(path.dirname(distPath), { recursive: true }); + yield copyFile(buildPath, distPath); + yield copyFile(sourcemapDir, mapPath); yield updateSourceMapURL(distPath, sourcemapName); let manifest = yield import(distPath); From 7f1b18a06b55ee451d39ea0513ad8723c177c646 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Sun, 12 Jul 2020 13:04:50 -0400 Subject: [PATCH 06/11] Use ensure() API in Bundler rather than try/catch --- packages/bundler/src/bundler.ts | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index ebfc36c37..93df6388f 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -2,6 +2,7 @@ import { Operation, resource } from 'effection'; import { on } from '@effection/events'; import { subscribe, Subscribable, SymbolSubscribable, ChainableSubscription } from '@effection/subscription'; import { Channel } from '@effection/channel'; +import { ensure } from '@bigtest/effection'; import { watch, RollupWatchOptions, RollupWatcherEvent, RollupWatcher } from 'rollup'; import resolve from '@rollup/plugin-node-resolve'; import * as commonjs from '@rollup/plugin-commonjs'; @@ -71,22 +72,22 @@ export class Bundler implements Subscribable { return yield resource(bundler, function*() { let rollup: RollupWatcher | null = null; - try { - rollup = watch(prepareRollupOptions(bundles)); - let events: ChainableSubscription, undefined> = yield subscribe(on>(rollup, 'event')); - let messages = events - .map(([event]) => event) - .filter(event => event.code === 'END' || event.code === 'ERROR') - .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); - - yield messages.forEach(function*(message) { - bundler.channel.send(message as BundlerMessage); - }); - } finally { + yield ensure(() => { if (rollup) { rollup.close(); } - } + }); + + rollup = watch(prepareRollupOptions(bundles)); + let events: ChainableSubscription, undefined> = yield subscribe(on>(rollup, 'event')); + let messages = events + .map(([event]) => event) + .filter(event => event.code === 'END' || event.code === 'ERROR') + .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); + + yield messages.forEach(function*(message) { + bundler.channel.send(message as BundlerMessage); + }); }); } From 7063bce3607b38f2312044847671294f50c27e0f Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Sun, 12 Jul 2020 13:12:53 -0400 Subject: [PATCH 07/11] Add changesets --- .changeset/bundler-shutdown.md | 5 +++++ .changeset/manifest-builder-warnings.md | 5 +++++ .changeset/manifest-watcher.md | 5 +++++ 3 files changed, 15 insertions(+) create mode 100644 .changeset/bundler-shutdown.md create mode 100644 .changeset/manifest-builder-warnings.md create mode 100644 .changeset/manifest-watcher.md diff --git a/.changeset/bundler-shutdown.md b/.changeset/bundler-shutdown.md new file mode 100644 index 000000000..f578e77dc --- /dev/null +++ b/.changeset/bundler-shutdown.md @@ -0,0 +1,5 @@ +--- +"@bigtest/bundler": patch +--- + +The `try`/`finally` in `Bundler` was not wrapping the code that it should have; switch to using `ensure()` API. \ No newline at end of file diff --git a/.changeset/manifest-builder-warnings.md b/.changeset/manifest-builder-warnings.md new file mode 100644 index 000000000..5e0c17150 --- /dev/null +++ b/.changeset/manifest-builder-warnings.md @@ -0,0 +1,5 @@ +--- +"@bigtest/server": patch +--- + +Work around bug in node.js that throws warnings when using `fs.promises.truncate()`: https://github.com/nodejs/node/issues/34189 diff --git a/.changeset/manifest-watcher.md b/.changeset/manifest-watcher.md new file mode 100644 index 000000000..93bf432ef --- /dev/null +++ b/.changeset/manifest-watcher.md @@ -0,0 +1,5 @@ +--- +"@bigtest/bundler": patch +--- + +Properly ignore `node_modules` from `Bundler` file watcher. From 5ffbefcad0734ccf1786800c205151b93941ca91 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Mon, 13 Jul 2020 23:11:16 -0400 Subject: [PATCH 08/11] Refactor control function to Deferred --- packages/server/src/manifest-builder.ts | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index 7c9f0eea5..d2d3dc76c 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -2,7 +2,7 @@ import { bigtestGlobals } from '@bigtest/globals'; import { Operation } from 'effection'; import { once } from '@effection/events'; import { subscribe, ChainableSubscription } from '@effection/subscription'; -import { Mailbox, ensure } from '@bigtest/effection'; +import { Mailbox, ensure, Deferred } from '@bigtest/effection'; import { Bundler, BundlerMessage, BundlerError } from '@bigtest/bundler'; import { Atom } from '@bigtest/atom'; import { createFingerprint } from 'fprint'; @@ -24,16 +24,18 @@ interface ManifestBuilderOptions { distDir: string; }; -function ftruncate(fd: number, len: number): Operation { - return ({ fail, resume }) => { - fs.ftruncate(fd, len, err => { - if (err) { - fail(err); - return; - } - resume(); - }) - }; +function* ftruncate(fd: number, len: number): Operation { + let { resolve, reject, promise } = Deferred(); + + fs.ftruncate(fd, len, err => { + if (err) { + reject(err); + } else { + resolve(); + } + }); + + yield promise; } // https://github.com/nodejs/node/issues/34189#issuecomment-654878715 From a19a8a87003ac40feb3e26a1d7959d452d3c8db7 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Mon, 13 Jul 2020 23:13:06 -0400 Subject: [PATCH 09/11] Refactor ensure to try/finally --- packages/server/src/manifest-builder.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/server/src/manifest-builder.ts b/packages/server/src/manifest-builder.ts index d2d3dc76c..14ef006af 100644 --- a/packages/server/src/manifest-builder.ts +++ b/packages/server/src/manifest-builder.ts @@ -2,7 +2,7 @@ import { bigtestGlobals } from '@bigtest/globals'; import { Operation } from 'effection'; import { once } from '@effection/events'; import { subscribe, ChainableSubscription } from '@effection/subscription'; -import { Mailbox, ensure, Deferred } from '@bigtest/effection'; +import { Mailbox, Deferred } from '@bigtest/effection'; import { Bundler, BundlerMessage, BundlerError } from '@bigtest/bundler'; import { Atom } from '@bigtest/atom'; import { createFingerprint } from 'fprint'; @@ -41,8 +41,12 @@ function* ftruncate(fd: number, len: number): Operation { // https://github.com/nodejs/node/issues/34189#issuecomment-654878715 function* truncate(path: string, len: number): Operation { let file: fs.promises.FileHandle = yield open(path, 'r+'); - yield ensure(() => file.close()); - yield ftruncate(file.fd, len); + + try { + yield ftruncate(file.fd, len); + } finally { + file.close(); + } } export function* updateSourceMapURL(filePath: string, sourcemapName: string): Operation{ From 53d8d605266d6a3a6fb8739744b4b88fd1e15ffa Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Tue, 14 Jul 2020 00:52:17 -0400 Subject: [PATCH 10/11] Refactor another ensure to try/finally --- packages/bundler/src/bundler.ts | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index 93df6388f..2eabbd011 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -70,24 +70,21 @@ export class Bundler implements Subscribable { let bundler = new Bundler(); return yield resource(bundler, function*() { - let rollup: RollupWatcher | null = null; + let rollup: RollupWatcher = watch(prepareRollupOptions(bundles));; - yield ensure(() => { - if (rollup) { - rollup.close(); - } - }); + try { + let events: ChainableSubscription, undefined> = yield subscribe(on>(rollup, 'event')); + let messages = events + .map(([event]) => event) + .filter(event => event.code === 'END' || event.code === 'ERROR') + .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); - rollup = watch(prepareRollupOptions(bundles)); - let events: ChainableSubscription, undefined> = yield subscribe(on>(rollup, 'event')); - let messages = events - .map(([event]) => event) - .filter(event => event.code === 'END' || event.code === 'ERROR') - .map(event => event.code === 'ERROR' ? { type: 'error', error: event.error } : { type: 'update' }); - - yield messages.forEach(function*(message) { - bundler.channel.send(message as BundlerMessage); - }); + yield messages.forEach(function*(message) { + bundler.channel.send(message as BundlerMessage); + }); + } finally { + rollup.close(); + } }); } From 5e0adf7476ca2210d8e77971b97b792ce8eeab57 Mon Sep 17 00:00:00 2001 From: Robbie Pitts Date: Tue, 14 Jul 2020 01:12:47 -0400 Subject: [PATCH 11/11] Fix lint error --- .changeset/bundler-shutdown.md | 2 +- packages/bundler/src/bundler.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/.changeset/bundler-shutdown.md b/.changeset/bundler-shutdown.md index f578e77dc..853a1f8b2 100644 --- a/.changeset/bundler-shutdown.md +++ b/.changeset/bundler-shutdown.md @@ -2,4 +2,4 @@ "@bigtest/bundler": patch --- -The `try`/`finally` in `Bundler` was not wrapping the code that it should have; switch to using `ensure()` API. \ No newline at end of file +The `try`/`finally` in `Bundler` was not wrapping the code that it should have; fix this by wrapping more. \ No newline at end of file diff --git a/packages/bundler/src/bundler.ts b/packages/bundler/src/bundler.ts index 2eabbd011..2efcb4c6c 100644 --- a/packages/bundler/src/bundler.ts +++ b/packages/bundler/src/bundler.ts @@ -2,7 +2,6 @@ import { Operation, resource } from 'effection'; import { on } from '@effection/events'; import { subscribe, Subscribable, SymbolSubscribable, ChainableSubscription } from '@effection/subscription'; import { Channel } from '@effection/channel'; -import { ensure } from '@bigtest/effection'; import { watch, RollupWatchOptions, RollupWatcherEvent, RollupWatcher } from 'rollup'; import resolve from '@rollup/plugin-node-resolve'; import * as commonjs from '@rollup/plugin-commonjs';