From 3205c17c412d44bb59842381c8a9226069040e55 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 14:26:13 -0700 Subject: [PATCH 01/14] Document soft dependencies --- CHANGELOG.md | 49 +++++++++++++++++++++++++++++++++- README.md | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1964f3612..43377a69f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,54 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). - +## [Unreleased] + +### Added + +- Added _soft dependencies_. + + By default, the cache key of a script includes the cache keys of its + dependencies. This means a script will re-run whenever one of its dependencies + re-runs, even if the output produced by the dependency didn't actually change. + + Now, if a dependency is annotated with `"soft": true`, then the cache key of + that dependency will no longer be included in the script's own cache key. This + means a script won't neccessarily re-run just because a dependency re-ran — + though Wireit will still always run the dependency first if it is not + up-to-date. + + Using `"soft": true` can result in faster builds thanks to fewer re-runs, but + it is very important to specify all of the input files generated by the + dependency which the script depends on in the `files` array. + + Example: + + ```json + { + "wireit": { + "build": { + "command": "tsc", + "files": ["tsconfig.json", "src/**/*.ts"], + "output": "lib/**", + }, + "bundle": { + "command": "rollup -c", + "files": ["rollup.config.json", "lib/**/*.js", "!lib/test"], + "output": "dist/bundle.js", + "dependencies": { + [ + "script": "build", + "soft": true + ] + } + } + } + } + ``` + +### Changed + +- Added length > 0 requirement to `schema.json`. ## [0.4.3] - 2022-05-15 diff --git a/README.md b/README.md index 6efa872aa..94503c381 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,7 @@ - [Dependencies](#dependencies) - [Vanilla scripts](#vanilla-scripts) - [Cross-package dependencies](#cross-package-dependencies) + - [Soft dependencies](#soft-dependencies) - [Parallelism](#parallelism) - [Input and output files](#input-and-output-files) - [Incremental build](#incremental-build) @@ -169,6 +170,79 @@ workspaces, as well as in other kinds of monorepos. } ``` +### Soft dependencies + +By default, if script A depends on script B, then script B's cache key is +automatically included in script A's cache key. That means if an input file for +script B changes, which causes B to re-run, then script A will re-run too — +_regardless of whether script B actually emitted different output._ + +This is a safe default because it means you aren't required to specify the input +files for every script when those input files are generated by a dependency; +Wireit assumes that any time script B runs, script A could be affected. However, +it comes with the tradeoff that scripts will sometimes re-run even when none of +their input files changed. + +To change this behavior and further optimize your build, you can specify that a +dependency is _soft_. This prevents the cache key of the dependency from being +inherited. Wireit will still ensure that the dependency is up-to-date, but it +won't assume that a script needs to re-run just because its dependency did. + +To declare a soft dependency, create an object for your dependency instead of a +plain string, and set `soft` to `true`: + +```json +{ + "wireit": { + "A": { + "dependencies": [ + { + "script": "B", + "soft": true + } + ] + } + } +} +``` + +In the following example, `bundle` has a `soft` dependency on `build`. +Importantly, it also includes `lib/**/*.js` in its `files` array, which are the +specific outputs from `tsc` that `rollup` consumes. Including these input files +wasn't neccessary before, but with `soft` it is now critical. + +```json +{ + "scripts": { + "build": "wireit", + "bundle": "wireit" + }, + "wireit": { + "build": { + "command": "tsc", + "files": ["src/**/*.ts", "tsconfig.json"], + "output": ["lib/**"] + }, + "bundle": { + "command": "rollup -c", + "dependencies": [ + { + "script": "build", + "soft": true + } + ], + "files": ["rollup.config.json", "lib/**/*.js", "!lib/test"], + "output": ["dist/bundle.js"] + } + } +} +``` + +The advantage of this configuration is that if `tsc` re-runs but doesn't produce +different `.js` files (for example, if it only produced different `.d.ts` +files), then `rollup` won't need to re-run. We've also excluded the `lib/test` +directory, because we know test files aren't included in our bundles. + ## Parallelism Wireit will run scripts in parallel whenever it is safe to do so according to From 8d7700e430c9d1e8428ae1e9a185530402b01f02 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 14:44:56 -0700 Subject: [PATCH 02/14] Add "soft" to schema.json and also add minLength requirements --- CHANGELOG.md | 3 +- schema.json | 34 ++++++++-- src/test/json-schema.test.ts | 120 +++++++++++++++++++++++++++++++++- src/test/util/package-json.ts | 2 +- 4 files changed, 150 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 43377a69f..685fc0e36 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -53,7 +53,8 @@ Versioning](https://semver.org/spec/v2.0.0.html). ### Changed -- Added length > 0 requirement to `schema.json`. +- Added string length > 0 requirement to the `command`, `dependencies`, `files`, + `output`, and `packageLocks` properties in `schema.json`. ## [0.4.3] - 2022-05-15 diff --git a/schema.json b/schema.json index 6848f6e39..09cef42d5 100644 --- a/schema.json +++ b/schema.json @@ -14,33 +14,57 @@ }, "command": { "markdownDescription": "The command to run.\n\nThis is a shell command that will be executed, with all binaries from npm dependencies and devDependencies available.\n\nFor example:\n\n```json\n\"command\": \"tsc\"\n```\n\nFor more info, see https://docs.npmjs.com/cli/v8/using-npm/scripts#environment", - "type": "string" + "type": "string", + "minLength": 1 }, "dependencies": { "markdownDescription": "Other npm scripts that will run before this one.\n\nThese scripts do not have to use wireit.\n\nDependencies can refer to scripts in other npm packages by using a relative path with the syntax `:`. All cross-package dependencies should start with a `\".\"`. Cross-package dependencies work well for npm workspaces, as well as in other kinds of monorepos.\n\nFor example:\n\n```json\n\"dependencies\": [\n \"build\",\n \"./packages/foo:build\"\n]\n```\n\nFor more info, see https://github.com/google/wireit#dependencies", "items": { - "type": "string" + "anyOf": [ + { + "type": "string", + "minLength": 1 + }, + { + "type": "object", + "required": ["script"], + "properties": { + "script": { + "markdownDescription": "The name of the script (see `dependencies`).", + "type": "string", + "minLength": 1 + }, + "soft": { + "markdownDescription": "If `false` (the default), the cache key of the dependency is automatically included in this script's cache key. This means that whenever the dependency re-runs, this script will re-run too, even if the output produced by the dependency didn't change.\n\nIf `true`, Wireit won't assume that this script needs to re-run just because the dependency re-ran. Instead, the dependency will still run first and be kept up-to-date, but whether this script runs is entirely determined by `files`. Be sure that any input files this script needs from the dependency are specified in `files`.\n\nFor more info, see https://github.com/google/wireit#soft-dependencies", + "enum": [true, false] + } + } + } + ] }, "type": "array" }, "files": { "markdownDescription": "The files that this script depends on.\n\nThese are the files that are watched when run with the `watch` argument. They are also used to determine if a script is stale or if its files and dependencies haven't changed and execution can be skipped.\n\nDon't specify `files` unless the array of files (and `dependencies`) are the only things that this script depends on. For example, a script that fetches data over the internet should not have a files array.\n\nThis should be a list of package-relative paths to files, or glob patterns. See https://github.com/google/wireit#glob-patterns for more info on the format of glob patterns.\n\nFor example:\n\n```json\n\"files\": [\n \"src/**/*.ts\"\n]\n```", "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "type": "array" }, "output": { "markdownDescription": "The files that this script writes.\n\nThese are the files that are deleted before the script is executed (set `clean` to customize this behavior), and these are the files that are cached if `files` is specified.\n\nThis should be a list of package-relative paths to files, or glob patterns. See https://github.com/google/wireit#glob-patterns for more info on the format of glob patterns.\n\nFor example:\n\n```json\n\"output\": [\n \"lib/**/*\",\n \"!lib/bundle.js\"\n]\n```", "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "type": "array" }, "packageLocks": { "markdownDescription": "By default, Wireit automatically treats package-lock.json files in the package directory, plus all parent directories, as input files. This is useful because installing or upgrading your dependencies can affect the behavior of your scripts, so it's important to re-run them whenever your dependencies change.\n\nIf you are using an alternative package manager instead of npm, then your package lock files might be named something else.\n\nFor more info, see: https://github.com/google/wireit#package-locks", "items": { - "type": "string" + "type": "string", + "minLength": 1 }, "type": "array" } diff --git a/src/test/json-schema.test.ts b/src/test/json-schema.test.ts index afab20989..8f131e34a 100644 --- a/src/test/json-schema.test.ts +++ b/src/test/json-schema.test.ts @@ -58,6 +58,14 @@ test('a script with just dependencies is valid', () => { shouldValidate({wireit: {a: {dependencies: ['b']}}}); }); +test('dependency object is valid', () => { + shouldValidate({wireit: {a: {dependencies: [{script: 'b'}]}}}); +}); + +test('dependency object with soft annotation is valid', () => { + shouldValidate({wireit: {a: {dependencies: [{script: 'b', soft: true}]}}}); +}); + // I couldn't figure out how to make this test pass while keeping the other // error messages reasonable. // It just turned all errors into this one. @@ -72,7 +80,7 @@ test('a script with all fields set is valid', () => { wireit: { a: { command: 'b', - dependencies: ['c'], + dependencies: ['c', {script: 'c', soft: true}], files: ['d'], output: ['e'], clean: true, @@ -122,6 +130,80 @@ test('clean can be either a boolean or the string if-file-deleted', () => { ); }); +test('command must not be empty', () => { + expectValidationErrors( + { + wireit: { + a: { + command: '', + }, + }, + }, + ['instance.wireit.a.command does not meet minimum length of 1'] + ); +}); + +test('dependencies[i] must not be empty', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'true', + dependencies: [''], + }, + }, + }, + // TODO(aomarks) Can we get a better error message? Seems like the built-in + // toString() doesn't recurse, so we'd have to build the whole error message + // ourselves. + [ + 'instance.wireit.a.dependencies[0] is not any of [subschema 0],[subschema 1]', + ] + ); +}); + +test('files[i] must not be empty', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'true', + files: [''], + }, + }, + }, + ['instance.wireit.a.files[0] does not meet minimum length of 1'] + ); +}); + +test('output[i] must not be empty', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'true', + output: [''], + }, + }, + }, + ['instance.wireit.a.output[0] does not meet minimum length of 1'] + ); +}); + +test('packageLocks[i] must not be empty', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'true', + packageLocks: [''], + }, + }, + }, + ['instance.wireit.a.packageLocks[0] does not meet minimum length of 1'] + ); +}); + test('dependencies must be an array of strings', () => { expectValidationErrors( { @@ -144,7 +226,41 @@ test('dependencies must be an array of strings', () => { }, }, }, - ['instance.wireit.a.dependencies[0] is not of a type(s) string'] + [ + 'instance.wireit.a.dependencies[0] is not any of [subschema 0],[subschema 1]', + ] + ); +}); + +test('dependencies[i].script is required', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'b', + dependencies: [{}], + }, + }, + }, + [ + 'instance.wireit.a.dependencies[0] is not any of [subschema 0],[subschema 1]', + ] + ); +}); + +test('dependencies[i].soft must be boolean', () => { + expectValidationErrors( + { + wireit: { + a: { + command: 'b', + dependencies: [{script: 'b', soft: 1}], + }, + }, + }, + [ + 'instance.wireit.a.dependencies[0] is not any of [subschema 0],[subschema 1]', + ] ); }); diff --git a/src/test/util/package-json.ts b/src/test/util/package-json.ts index d017dc814..250bb2d7c 100644 --- a/src/test/util/package-json.ts +++ b/src/test/util/package-json.ts @@ -16,7 +16,7 @@ export interface PackageJson { wireit?: { [scriptName: string]: { command?: string; - dependencies?: string[]; + dependencies?: Array; files?: string[]; output?: string[]; clean?: boolean | 'if-file-deleted'; From a223dd22bc8e907e65baa24085509c4f6c5a9c95 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 15:45:21 -0700 Subject: [PATCH 03/14] Allow dependency to be an object --- src/analyzer.ts | 70 ++++++++++++++++-- src/test/basic.test.ts | 47 ++++++++++++ src/test/errors-analysis.test.ts | 122 ++++++++++++++++++++++++++++++- 3 files changed, 231 insertions(+), 8 deletions(-) diff --git a/src/analyzer.ts b/src/analyzer.ts index b06cfa523..7feeed0f9 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -481,17 +481,73 @@ export class Analyzer { const uniqueDependencies = new Map(); const children = dependenciesAst.children ?? []; for (let i = 0; i < children.length; i++) { + // A dependency can be either a plain string, or an object with a "script" + // property plus optional extra annotations. const maybeUnresolved = children[i]; - const stringResult = failUnlessNonBlankString( - maybeUnresolved, - packageJson.jsonFile - ); - if (!stringResult.ok) { + let specifierResult; + if (maybeUnresolved.type === 'string') { + specifierResult = failUnlessNonBlankString( + maybeUnresolved, + packageJson.jsonFile + ); + if (!specifierResult.ok) { + encounteredError = true; + placeholder.failures.push(specifierResult.error); + continue; + } + } else if (maybeUnresolved.type === 'object') { + specifierResult = findNodeAtLocation(maybeUnresolved, ['script']); + if (specifierResult == null) { + encounteredError = true; + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: {packageDir: pathlib.dirname(packageJson.jsonFile.path)}, + diagnostic: { + severity: 'error', + message: `Dependency object must set a "script" property.`, + location: { + file: packageJson.jsonFile, + range: { + offset: maybeUnresolved.offset, + length: maybeUnresolved.length, + }, + }, + }, + }); + continue; + } + specifierResult = failUnlessNonBlankString( + specifierResult, + packageJson.jsonFile + ); + if (!specifierResult.ok) { + encounteredError = true; + placeholder.failures.push(specifierResult.error); + continue; + } + } else { encounteredError = true; - placeholder.failures.push(stringResult.error); + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: {packageDir: pathlib.dirname(packageJson.jsonFile.path)}, + diagnostic: { + severity: 'error', + message: `Expected a string or object, but was ${maybeUnresolved.type}.`, + location: { + file: packageJson.jsonFile, + range: { + offset: maybeUnresolved.offset, + length: maybeUnresolved.length, + }, + }, + }, + }); continue; } - const unresolved = stringResult.value; + + const unresolved = specifierResult.value; const result = this.#resolveDependency( unresolved, placeholder, diff --git a/src/test/basic.test.ts b/src/test/basic.test.ts index 8a50e5aa1..d8812aeaa 100644 --- a/src/test/basic.test.ts +++ b/src/test/basic.test.ts @@ -363,6 +363,53 @@ test( }) ); +test( + 'cross-package dependency using object format', + timeout(async ({rig}) => { + const cmdA = await rig.newCommand(); + const cmdB = await rig.newCommand(); + await rig.write({ + 'foo/package.json': { + scripts: { + a: 'wireit', + }, + wireit: { + a: { + command: cmdA.command, + dependencies: [ + { + script: '../bar:b', + }, + ], + }, + }, + }, + 'bar/package.json': { + scripts: { + b: 'wireit', + }, + wireit: { + b: { + command: cmdB.command, + }, + }, + }, + }); + const exec = rig.exec('npm run a', {cwd: 'foo'}); + + const invB = await cmdB.nextInvocation(); + invB.exit(0); + + const invA = await cmdA.nextInvocation(); + invA.exit(0); + + const res = await exec.exit; + assert.equal(res.code, 0); + assert.equal(cmdA.numInvocations, 1); + assert.equal(cmdB.numInvocations, 1); + }) +); + test( 'cross-package dependency that validly cycles back to the first package', timeout(async ({rig}) => { diff --git a/src/test/errors-analysis.test.ts b/src/test/errors-analysis.test.ts index 5f48d0bb5..57ca5384c 100644 --- a/src/test/errors-analysis.test.ts +++ b/src/test/errors-analysis.test.ts @@ -199,13 +199,40 @@ test( assertScriptOutputEquals( done.stderr, ` -❌ package.json:8:9 Expected a string, but was array. +❌ package.json:8:9 Expected a string or object, but was array. [] ~~` ); }) ); +test(`dependencies.script is not a string (object form)`, async ({rig}) => { + await rig.write('package.json', { + scripts: { + a: 'wireit', + }, + wireit: { + a: { + dependencies: [ + { + script: [], + }, + ], + }, + }, + }); + const execResult = rig.exec(`npm run a`); + const done = await execResult.exit; + assert.equal(done.code, 1); + assertScriptOutputEquals( + done.stderr, + ` +❌ package.json:9:21 Expected a string, but was array. + "script": [] + ~~` + ); +}); + test( 'dependency is empty or blank', timeout(async ({rig}) => { @@ -234,6 +261,66 @@ test( }) ); +test(`dependencies.script is empty or blank (object form)`, async ({rig}) => { + await rig.write('package.json', { + scripts: { + a: 'wireit', + 1: 'wireit', + }, + wireit: { + a: { + command: 'true', + dependencies: [ + { + script: '', + }, + ], + }, + 1: { + command: 'true', + }, + }, + }); + const execResult = rig.exec(`npm run a`); + const done = await execResult.exit; + assert.equal(done.code, 1); + assertScriptOutputEquals( + done.stderr, + ` +❌ package.json:14:21 Expected this field to be nonempty + "script": "" + ~~` + ); +}); + +test(`dependencies.script is missing (object form)`, async ({rig}) => { + await rig.write('package.json', { + scripts: { + a: 'wireit', + 1: 'wireit', + }, + wireit: { + a: { + command: 'true', + dependencies: [{}], + }, + 1: { + command: 'true', + }, + }, + }); + const execResult = rig.exec(`npm run a`); + const done = await execResult.exit; + assert.equal(done.code, 1); + assertScriptOutputEquals( + done.stderr, + ` +❌ package.json:13:9 Dependency object must set a "script" property. + {} + ~~` + ); +}); + test( 'command is not a string', timeout(async ({rig}) => { @@ -670,6 +757,39 @@ test( }) ); +test( + 'missing cross package dependency (object form)', + timeout(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + }, + wireit: { + a: { + dependencies: [{script: './child:missing'}], + }, + }, + }, + 'child/package.json': { + scripts: {}, + }, + }); + const result = rig.exec('npm run a'); + const done = await result.exit; + assert.equal(done.code, 1); + assertScriptOutputEquals( + done.stderr, + ` +❌ package.json:9:30 Cannot find script named "missing" in package "${rig.resolve( + 'child' + )}" + "script": "./child:missing" + ~~~~~~~` + ); + }) +); + test( 'missing same-package dependency with colon in name', timeout(async ({rig}) => { From a0335be001da860fca777ffccd1ef0dd3df5ffc4 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 16:05:29 -0700 Subject: [PATCH 04/14] Extract and validate "soft" property --- src/analyzer.ts | 35 +++++++++++++++++++++++++++--- src/ide.ts | 4 ++-- src/script.ts | 3 ++- src/test/errors-analysis.test.ts | 37 ++++++++++++++++++++++++++++++++ 4 files changed, 73 insertions(+), 6 deletions(-) diff --git a/src/analyzer.ts b/src/analyzer.ts index 7feeed0f9..dd6a16f66 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -485,6 +485,7 @@ export class Analyzer { // property plus optional extra annotations. const maybeUnresolved = children[i]; let specifierResult; + let soft = false; // Default; if (maybeUnresolved.type === 'string') { specifierResult = failUnlessNonBlankString( maybeUnresolved, @@ -526,6 +527,31 @@ export class Analyzer { placeholder.failures.push(specifierResult.error); continue; } + const softResult = findNodeAtLocation(maybeUnresolved, ['soft']); + if (softResult !== undefined) { + if (softResult.value === true || softResult.value === false) { + soft = softResult.value; + } else { + encounteredError = true; + placeholder.failures.push({ + type: 'failure', + reason: 'invalid-config-syntax', + script: {packageDir: pathlib.dirname(packageJson.jsonFile.path)}, + diagnostic: { + severity: 'error', + message: `The "soft" property must be either true or false.`, + location: { + file: packageJson.jsonFile, + range: { + offset: softResult.offset, + length: softResult.length, + }, + }, + }, + }); + continue; + } + } } else { encounteredError = true; placeholder.failures.push({ @@ -597,8 +623,9 @@ export class Analyzer { uniqueDependencies.set(uniqueKey, unresolved); const placeHolderInfo = this.#getPlaceholder(resolved); dependencies.push({ - astNode: unresolved, + specifier: unresolved, config: placeHolderInfo.placeholder, + soft, }); this.#ongoingWorkPromises.push( (async () => { @@ -905,7 +932,9 @@ export class Analyzer { // Use the actual value in the array, because this could refer to // a script in another package. const nextName = - nextNode?.astNode?.value ?? next?.name ?? trailArray[cycleStart].name; + nextNode?.specifier?.value ?? + next?.name ?? + trailArray[cycleStart].name; const message = next === trailArray[cycleStart] ? `${JSON.stringify(current.name)} points back to ${JSON.stringify( @@ -917,7 +946,7 @@ export class Analyzer { const culpritNode = // This should always be present - nextNode?.astNode ?? + nextNode?.specifier ?? // But failing that, fall back to the best node we have. current.configAstNode?.name ?? current.scriptAstNode?.name; diff --git a/src/ide.ts b/src/ide.ts index b1814a327..7bbabd091 100644 --- a/src/ide.ts +++ b/src/ide.ts @@ -305,7 +305,7 @@ export class IdeAnalyzer { return [ { originSelectionRange: sourceConverter.toIdeRange( - scriptInfo.dependency.astNode + scriptInfo.dependency.specifier ), targetUri: url.pathToFileURL(targetFile.path).toString(), targetRange: targetConverter.toIdeRange( @@ -365,7 +365,7 @@ export class IdeAnalyzer { packageDir: pathlib.dirname(packageJson.jsonFile.path), }); for (const dep of script.dependencies ?? []) { - if (offsetInsideRange(offset, dep.astNode)) { + if (offsetInsideRange(offset, dep.specifier)) { return { kind: 'dependency' as const, dependency: dep, diff --git a/src/script.ts b/src/script.ts index 8f1844407..bd489bade 100644 --- a/src/script.ts +++ b/src/script.ts @@ -31,7 +31,8 @@ export interface ScriptReference extends PackageReference { export interface Dependency { config: Config; - astNode: JsonAstNode; + specifier: JsonAstNode; + soft: boolean; } /** diff --git a/src/test/errors-analysis.test.ts b/src/test/errors-analysis.test.ts index 57ca5384c..cf1d77d14 100644 --- a/src/test/errors-analysis.test.ts +++ b/src/test/errors-analysis.test.ts @@ -1817,4 +1817,41 @@ test(`repro an issue with looking for a colon in missing dependency`, async ({ ); }); +test( + 'dependencies.soft is not a boolean', + timeout(async ({rig}) => { + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + b: 'wireit', + }, + wireit: { + a: { + dependencies: [ + { + script: 'b', + soft: 1, + }, + ], + }, + b: { + command: 'true', + }, + }, + }, + }); + const result = rig.exec('npm run a'); + const done = await result.exit; + assert.equal(done.code, 1); + assertScriptOutputEquals( + done.stderr, + ` +❌ package.json:11:19 The "soft" property must be either true or false. + "soft": 1 + ~` + ); + }) +); + test.run(); From 4f78a1da7915f06cde6aed15d3e50394a839ebd4 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 16:21:31 -0700 Subject: [PATCH 05/14] Add test for soft dependencies --- src/test/basic.test.ts | 89 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/src/test/basic.test.ts b/src/test/basic.test.ts index d8812aeaa..83b09eba9 100644 --- a/src/test/basic.test.ts +++ b/src/test/basic.test.ts @@ -986,4 +986,93 @@ test( }) ); +test( + 'soft dependency does not inherit cache key', + timeout(async ({rig}) => { + // a --[soft]--> b --> c + const a = await rig.newCommand(); + const b = await rig.newCommand(); + const c = await rig.newCommand(); + await rig.write({ + 'package.json': { + scripts: { + a: 'wireit', + b: 'wireit', + c: 'wireit', + }, + wireit: { + a: { + command: a.command, + dependencies: [ + { + script: 'b', + soft: true, + }, + ], + files: ['inputs/a'], + }, + b: { + command: b.command, + dependencies: ['c'], + files: ['inputs/b'], + }, + c: { + command: c.command, + files: ['inputs/c'], + }, + }, + }, + }); + + // Initially everything runs. + { + await rig.write('inputs/a', 'v1'); + await rig.write('inputs/b', 'v1'); + await rig.write('inputs/c', 'v1'); + const wireit = rig.exec('npm run a'); + (await c.nextInvocation()).exit(0); + (await b.nextInvocation()).exit(0); + (await a.nextInvocation()).exit(0); + assert.equal((await wireit.exit).code, 0); + assert.equal(a.numInvocations, 1); + assert.equal(b.numInvocations, 1); + assert.equal(c.numInvocations, 1); + } + + // Changing input of B re-runs B but not A. + { + await rig.write('inputs/b', 'v2'); + const wireit = rig.exec('npm run a'); + (await b.nextInvocation()).exit(0); + assert.equal((await wireit.exit).code, 0); + assert.equal(a.numInvocations, 1); + assert.equal(b.numInvocations, 2); + assert.equal(c.numInvocations, 1); + } + + // Changing input of C re-runs B and C but not A. + { + await rig.write('inputs/c', 'v2'); + const wireit = rig.exec('npm run a'); + (await c.nextInvocation()).exit(0); + (await b.nextInvocation()).exit(0); + assert.equal((await wireit.exit).code, 0); + assert.equal(a.numInvocations, 1); + assert.equal(b.numInvocations, 3); + assert.equal(c.numInvocations, 2); + } + + // Changing input of A re-runs A (just to be sure!). + { + await rig.write('inputs/a', 'v2'); + const wireit = rig.exec('npm run a'); + (await a.nextInvocation()).exit(0); + assert.equal((await wireit.exit).code, 0); + assert.equal(a.numInvocations, 2); + assert.equal(b.numInvocations, 3); + assert.equal(c.numInvocations, 2); + } + }) +); + test.run(); From 8e81d207341c59674eb13d8a7049bfe0a58f16a2 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 16:22:15 -0700 Subject: [PATCH 06/14] Implement soft dependencies --- src/executor.ts | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/executor.ts b/src/executor.ts index 36cb89ee6..dd5b87743 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -22,9 +22,9 @@ import {ScriptChildProcess} from './script-child-process.js'; import {Deferred} from './util/deferred.js'; import type { + Dependency, ScriptConfig, ScriptConfigWithRequiredCommand, - ScriptReference, ScriptReferenceString, ScriptState, ScriptStateString, @@ -310,7 +310,7 @@ class ScriptExecution { } async #executeScript( - dependencyStates: Array<[ScriptReference, ScriptState]> + dependencyStates: Array<[Dependency, ScriptState]> ): Promise { // Note we must wait for dependencies to finish before generating the cache // key, because a dependency could create or modify an input file to this @@ -457,7 +457,7 @@ class ScriptExecution { } async #executeDependencies(): Promise< - Result, Failure[]> + Result, ScriptState]>, Failure[]> > { // Randomize the order we execute dependencies to make it less likely for a // user to inadvertently depend on any specific order, which could indicate @@ -471,7 +471,7 @@ class ScriptExecution { }) ); const errors = new Set(); - const results: Array<[ScriptReference, ScriptState]> = []; + const results: Array<[Dependency, ScriptState]> = []; for (let i = 0; i < dependencyResults.length; i++) { const result = dependencyResults[i]; if (result.status === 'rejected') { @@ -488,10 +488,7 @@ class ScriptExecution { errors.add(error); } } else { - results.push([ - this.#script.dependencies[i].config, - result.value.value, - ]); + results.push([this.#script.dependencies[i], result.value.value]); } } } @@ -682,17 +679,24 @@ class ScriptExecution { * and the state of its dependencies. */ async #computeState( - dependencyStates: Array<[ScriptReference, ScriptState]> + dependencyStates: Array<[Dependency, ScriptState]> ): Promise { let allDependenciesAreCacheable = true; const filteredDependencyStates: Array< [ScriptReferenceString, ScriptState] > = []; for (const [dep, depState] of dependencyStates) { + if (dep.soft) { + // Soft dependencies aren't included in the cache key. + continue; + } if (!depState.cacheable) { allDependenciesAreCacheable = false; } - filteredDependencyStates.push([scriptReferenceToString(dep), depState]); + filteredDependencyStates.push([ + scriptReferenceToString(dep.config), + depState, + ]); } let fileHashes: Array<[string, Sha256HexDigest]>; From 29ed2d9a3e19656c535ddeb7e5e27e69b452fdf4 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 15 May 2022 17:41:26 -0700 Subject: [PATCH 07/14] Drop concurrency of exclusive lock test, because it sometimes takes > 60 seconds --- src/test/parallelism.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/parallelism.test.ts b/src/test/parallelism.test.ts index 4e34a6e7e..4cb927eae 100644 --- a/src/test/parallelism.test.ts +++ b/src/test/parallelism.test.ts @@ -246,7 +246,7 @@ test( // Start up simultaneous Wireit invocations for the same // script. - const concurrency = 25; + const concurrency = 10; const wireits = []; for (let i = 0; i < concurrency; i++) { wireits.push(rig.exec('npm run a')); From a085f5b2f765530ef9b8257a893b4f52000414d8 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 17 May 2022 08:37:17 -0700 Subject: [PATCH 08/14] Address PR comments (1) --- README.md | 24 +++++++++++------------- schema.json | 4 ++-- src/analyzer.ts | 2 +- src/executor.ts | 8 ++++---- src/script.ts | 6 ++++-- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 94503c381..304c54792 100644 --- a/README.md +++ b/README.md @@ -172,21 +172,19 @@ workspaces, as well as in other kinds of monorepos. ### Soft dependencies -By default, if script A depends on script B, then script B's cache key is -automatically included in script A's cache key. That means if an input file for -script B changes, which causes B to re-run, then script A will re-run too — -_regardless of whether script B actually emitted different output._ +By default, whenever a dependency runs, the script that depends on it will be +marked stale and need to re-run too — regardless of whether the dependency +actually produced new or relevant output. This is a safe default because it means you aren't required to specify the input -files for every script when those input files are generated by a dependency; -Wireit assumes that any time script B runs, script A could be affected. However, -it comes with the tradeoff that scripts will sometimes re-run even when none of -their input files changed. - -To change this behavior and further optimize your build, you can specify that a -dependency is _soft_. This prevents the cache key of the dependency from being -inherited. Wireit will still ensure that the dependency is up-to-date, but it -won't assume that a script needs to re-run just because its dependency did. +files for every script when those input files are generated by a dependency. +However, it comes with the tradeoff that scripts will sometimes re-run even when +none of their input files changed. + +To change this behavior and further optimize your build, you can use the `soft` +setting to tell Wireit that all of the input files needed from a dependency are +declared in the `files` array. Now, Wireit won't assume that a script was stale +just because its dependency ran. To declare a soft dependency, create an object for your dependency instead of a plain string, and set `soft` to `true`: diff --git a/schema.json b/schema.json index 09cef42d5..128d48408 100644 --- a/schema.json +++ b/schema.json @@ -35,8 +35,8 @@ "minLength": 1 }, "soft": { - "markdownDescription": "If `false` (the default), the cache key of the dependency is automatically included in this script's cache key. This means that whenever the dependency re-runs, this script will re-run too, even if the output produced by the dependency didn't change.\n\nIf `true`, Wireit won't assume that this script needs to re-run just because the dependency re-ran. Instead, the dependency will still run first and be kept up-to-date, but whether this script runs is entirely determined by `files`. Be sure that any input files this script needs from the dependency are specified in `files`.\n\nFor more info, see https://github.com/google/wireit#soft-dependencies", - "enum": [true, false] + "markdownDescription": "When `false` (the default), whenever this dependency runs, this script (the dependent) will be marked stale and need to re-run too, regardless of whether the dependency produced new or relevant output. When soft is `true` Wireit won't assume that the dependent is stale just because the dependency ran. This can reduce unnecessary re-building when `files` captures all of the relevant output of the dependency .\n\nFor more info, see https://github.com/google/wireit#soft-dependencies", + "type": "boolean" } } } diff --git a/src/analyzer.ts b/src/analyzer.ts index dd6a16f66..005982591 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -1034,7 +1034,7 @@ export class Analyzer { const validConfig: ScriptConfig = { ...config, state: 'valid', - dependencies: config.dependencies as Array>, + dependencies: config.dependencies as Array, }; // We want to keep the original reference, but get type checking that // the only difference between a ScriptConfig and a diff --git a/src/executor.ts b/src/executor.ts index dd5b87743..80c318068 100644 --- a/src/executor.ts +++ b/src/executor.ts @@ -310,7 +310,7 @@ class ScriptExecution { } async #executeScript( - dependencyStates: Array<[Dependency, ScriptState]> + dependencyStates: Array<[Dependency, ScriptState]> ): Promise { // Note we must wait for dependencies to finish before generating the cache // key, because a dependency could create or modify an input file to this @@ -457,7 +457,7 @@ class ScriptExecution { } async #executeDependencies(): Promise< - Result, ScriptState]>, Failure[]> + Result, Failure[]> > { // Randomize the order we execute dependencies to make it less likely for a // user to inadvertently depend on any specific order, which could indicate @@ -471,7 +471,7 @@ class ScriptExecution { }) ); const errors = new Set(); - const results: Array<[Dependency, ScriptState]> = []; + const results: Array<[Dependency, ScriptState]> = []; for (let i = 0; i < dependencyResults.length; i++) { const result = dependencyResults[i]; if (result.status === 'rejected') { @@ -679,7 +679,7 @@ class ScriptExecution { * and the state of its dependencies. */ async #computeState( - dependencyStates: Array<[Dependency, ScriptState]> + dependencyStates: Array<[Dependency, ScriptState]> ): Promise { let allDependenciesAreCacheable = true; const filteredDependencyStates: Array< diff --git a/src/script.ts b/src/script.ts index bd489bade..8da1edd3c 100644 --- a/src/script.ts +++ b/src/script.ts @@ -29,7 +29,9 @@ export interface ScriptReference extends PackageReference { name: string; } -export interface Dependency { +export interface Dependency< + Config extends PotentiallyValidScriptConfig = ScriptConfig +> { config: Config; specifier: JsonAstNode; soft: boolean; @@ -56,7 +58,7 @@ export interface ScriptConfig extends ScriptReference { * directory + script name, but the {@link Executor} then randomizes the order * during execution. */ - dependencies: Array>; + dependencies: Array; /** * Input file globs for this script. From 5f987758d13ca367ed438025d29624ea3150eec0 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 17 May 2022 09:01:30 -0700 Subject: [PATCH 09/14] Add (slightly wrong) test for jump-to-def from object dep --- src/test/ide.test.ts | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/test/ide.test.ts b/src/test/ide.test.ts index 0aa8d8546..ac1412bd8 100644 --- a/src/test/ide.test.ts +++ b/src/test/ide.test.ts @@ -322,6 +322,40 @@ test(`we jump to the scripts section for a vanilla script`, async ({rig}) => { }); }); +test('jump to definition from object style dependency', async ({rig}) => { + const ide = new IdeAnalyzer(); + await assertDefinition(ide, { + path: rig.resolve('package.json'), + contentsWithPipe: JSON.stringify( + { + scripts: { + a: 'wireit', + b: 'echo', + }, + wireit: { + a: { + dependencies: [{script: '|b'}], + }, + }, + }, + null, + 2 + ), + expected: { + target: ` + "b": "echo" + ~~~~~~~~~~~`, + targetSelection: ` + "b": "echo" + ~~~`, + // TODO(aomarks) The ~~~ is 2 spaces ahead of where it should be. + originSelection: ` + "script": "b" + ~~~`, + }, + }); +}); + test(`we don't get definitions for non-dep locations`, async ({rig}) => { const ide = new IdeAnalyzer(); await assertDefinition(ide, { From b4fe4771392dae4998b70182d11415c2e5b050c0 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 17 May 2022 12:58:02 -0700 Subject: [PATCH 10/14] Fix test indentation --- src/test/ide.test.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/test/ide.test.ts b/src/test/ide.test.ts index ac1412bd8..f45d449a2 100644 --- a/src/test/ide.test.ts +++ b/src/test/ide.test.ts @@ -348,9 +348,8 @@ test('jump to definition from object style dependency', async ({rig}) => { targetSelection: ` "b": "echo" ~~~`, - // TODO(aomarks) The ~~~ is 2 spaces ahead of where it should be. originSelection: ` - "script": "b" + "script": "b" ~~~`, }, }); From db7e259c1d735001649e00b8ebe40155e8ccde18 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Tue, 17 May 2022 13:40:10 -0700 Subject: [PATCH 11/14] Do undefined checks more precisely --- src/analyzer.ts | 22 +++++++++++----------- src/ide.ts | 19 +++++++++++-------- src/language-server.ts | 2 +- src/test/codeactions.test.ts | 4 ++-- src/test/ide.test.ts | 6 +++--- src/util/ast.ts | 2 +- src/util/package-json.ts | 20 ++++++++++---------- 7 files changed, 39 insertions(+), 36 deletions(-) diff --git a/src/analyzer.ts b/src/analyzer.ts index 005982591..f752a7837 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -212,7 +212,7 @@ export class Analyzer { for (const failure of failures) { const supercedes = (failure as Partial) .supercedes; - if (supercedes != null) { + if (supercedes !== undefined) { failures.delete(supercedes); } } @@ -292,7 +292,7 @@ export class Analyzer { if (syntaxInfo === undefined || syntaxInfo.scriptNode === undefined) { let node; let reason; - if (syntaxInfo?.wireitConfigNode?.name != null) { + if (syntaxInfo?.wireitConfigNode?.name !== undefined) { node = syntaxInfo.wireitConfigNode.name; reason = 'wireit-config-but-no-script' as const; } else { @@ -464,7 +464,7 @@ export class Analyzer { scriptInfo.wireitConfigNode && findNodeAtLocation(scriptInfo.wireitConfigNode, ['dependencies']); let encounteredError = false; - if (dependenciesAst == null) { + if (dependenciesAst === undefined) { return {dependencies, encounteredError}; } const result = failUnlessArray(dependenciesAst, packageJson.jsonFile); @@ -498,7 +498,7 @@ export class Analyzer { } } else if (maybeUnresolved.type === 'object') { specifierResult = findNodeAtLocation(maybeUnresolved, ['script']); - if (specifierResult == null) { + if (specifierResult === undefined) { encounteredError = true; placeholder.failures.push({ type: 'failure', @@ -706,7 +706,7 @@ export class Analyzer { packageJson: PackageJson, syntaxInfo: ScriptSyntaxInfo ): undefined | ArrayNode { - if (syntaxInfo.wireitConfigNode == null) { + if (syntaxInfo.wireitConfigNode === undefined) { return; } const filesNode = findNodeAtLocation(syntaxInfo.wireitConfigNode, [ @@ -739,7 +739,7 @@ export class Analyzer { packageJson: PackageJson, syntaxInfo: ScriptSyntaxInfo ): undefined | ArrayNode { - if (syntaxInfo.wireitConfigNode == null) { + if (syntaxInfo.wireitConfigNode === undefined) { return; } const outputNode = findNodeAtLocation(syntaxInfo.wireitConfigNode, [ @@ -772,7 +772,7 @@ export class Analyzer { packageJson: PackageJson, syntaxInfo: ScriptSyntaxInfo ): undefined | boolean | 'if-file-deleted' { - if (syntaxInfo.wireitConfigNode == null) { + if (syntaxInfo.wireitConfigNode === undefined) { return; } const clean = findNodeAtLocation(syntaxInfo.wireitConfigNode, ['clean']) as @@ -810,7 +810,7 @@ export class Analyzer { syntaxInfo: ScriptSyntaxInfo, files: undefined | ArrayNode ): void { - if (syntaxInfo.wireitConfigNode == null) { + if (syntaxInfo.wireitConfigNode === undefined) { return; } const packageLocksNode = findNodeAtLocation(syntaxInfo.wireitConfigNode, [ @@ -910,7 +910,7 @@ export class Analyzer { } const trailArray = [...trail].map((key) => { const placeholderInfo = this.#placeholders.get(key); - if (placeholderInfo == null) { + if (placeholderInfo === undefined) { throw new Error( `Internal error: placeholder not found for ${key} during cycle detection` ); @@ -987,7 +987,7 @@ export class Analyzer { }; return {ok: false, error: this.#markAsInvalid(config, failure)}; } - if (config.dependencies != null && config.dependencies.length > 0) { + if (config.dependencies.length > 0) { // Sorting means that if the user re-orders the same set of dependencies, // the trail we take in this walk remains the same, so any cycle error // message we might throw will have the same trail, too. This also helps @@ -1018,7 +1018,7 @@ export class Analyzer { } trail.delete(trailKey); } - if (dependencyStillUnvalidated != null) { + if (dependencyStillUnvalidated !== undefined) { // At least one of our dependencies was unvalidated, likely because it // had a syntax error or was missing necessary information. Therefore // we can't transition to valid either. diff --git a/src/ide.ts b/src/ide.ts index 7bbabd091..fef11debe 100644 --- a/src/ide.ts +++ b/src/ide.ts @@ -110,10 +110,10 @@ export class IdeAnalyzer { const openFiles = new Set(this.openFiles); for (const failure of await this.#analyzer.analyzeFiles([...openFiles])) { - if (failure.diagnostic != null) { + if (failure.diagnostic !== undefined) { addDiagnostic(failure.diagnostic); } - if (failure.diagnostics != null) { + if (failure.diagnostics !== undefined) { for (const diagnostic of failure.diagnostics) { addDiagnostic(diagnostic); } @@ -159,8 +159,8 @@ export class IdeAnalyzer { } = scriptInfo; if ( scriptInfo.kind === 'scripts-section-script' && - scriptNode != null && - wireitConfigNode == null + scriptNode !== undefined && + wireitConfigNode === undefined ) { const edit = getEdit(packageJson.jsonFile, [ {path: ['scripts', name], value: 'wireit'}, @@ -175,7 +175,10 @@ export class IdeAnalyzer { edit, }); } - if (scriptInfo.kind === 'wireit-section-script' && scriptNode == null) { + if ( + scriptInfo.kind === 'wireit-section-script' && + scriptNode === undefined + ) { const edit = getEdit(packageJson.jsonFile, [ {path: ['scripts', script.name], value: 'wireit'}, ]); @@ -194,8 +197,8 @@ export class IdeAnalyzer { } if ( - scriptNode == null || - wireitConfigNode == null || + scriptNode === undefined || + wireitConfigNode === undefined || scriptNode.value === 'wireit' ) { return codeActions; @@ -294,7 +297,7 @@ export class IdeAnalyzer { const dep = scriptInfo.dependency; const targetFile = dep.config.declaringFile; const targetNode = dep.config.configAstNode ?? dep.config.scriptAstNode; - if (targetFile == null || targetNode == null) { + if (targetFile === undefined || targetNode === undefined) { return; } diff --git a/src/language-server.ts b/src/language-server.ts index 9e62f268a..39276f798 100644 --- a/src/language-server.ts +++ b/src/language-server.ts @@ -118,7 +118,7 @@ documents.onDidClose((change) => { connection.onCodeAction(async (params) => { const document = documents.get(params.textDocument.uri); - if (document == null) { + if (document === undefined) { return []; } const path = url.fileURLToPath(document.uri); diff --git a/src/test/codeactions.test.ts b/src/test/codeactions.test.ts index 6c1ef4496..6a8b16de0 100644 --- a/src/test/codeactions.test.ts +++ b/src/test/codeactions.test.ts @@ -124,7 +124,7 @@ function applyEdit( before: string, action: CodeAction ): string { - if (action.edit == null) { + if (action.edit === undefined) { throw new Error(`Action ${action.title} had no edit`); } const edit = action.edit; @@ -132,7 +132,7 @@ function applyEdit( const filename = rig.resolve('package.json'); assert.equal(Object.keys(edit?.changes ?? {}), [filename]); const textEdits = edit?.changes?.[filename]; - if (textEdits == null) { + if (textEdits === undefined) { throw new Error(`Action ${action.title} had no edits for ${filename}`); } const converter = OffsetToPositionConverter.createUncachedForTest(before); diff --git a/src/test/ide.test.ts b/src/test/ide.test.ts index f45d449a2..2a3dab2e7 100644 --- a/src/test/ide.test.ts +++ b/src/test/ide.test.ts @@ -174,7 +174,7 @@ async function assertDefinition( options.contentsWithPipe.slice(offset + 1); ide.setOpenFileContents(options.path, contents); const sourceFile = await ide.getPackageJsonForTest(options.path); - if (sourceFile == null) { + if (sourceFile === undefined) { throw new Error(`could not get source file`); } const sourceConverter = OffsetToPositionConverter.get(sourceFile.jsonFile); @@ -201,7 +201,7 @@ async function assertDefinition( const targetFile = await ide.getPackageJsonForTest( url.fileURLToPath(definition.targetUri) ); - if (targetFile == null) { + if (targetFile === undefined) { throw new Error(`Could not load target file`); } const targetConverter = OffsetToPositionConverter.get(targetFile.jsonFile); @@ -225,7 +225,7 @@ async function assertDefinition( targetSelectionSquiggle, options.expected.targetSelection ); - if (definition.originSelectionRange == null) { + if (definition.originSelectionRange === undefined) { throw new Error(`No iriginSelectionRange returned`); } const sourceSelectionSquiggle = drawSquiggle( diff --git a/src/util/ast.ts b/src/util/ast.ts index 516c81362..7c7646078 100644 --- a/src/util/ast.ts +++ b/src/util/ast.ts @@ -74,7 +74,7 @@ export function findNamedNodeAtLocation( return {ok: true, value: undefined}; } const name = parent.children?.[0]; - if (parent.type !== 'property' || name == null) { + if (parent.type !== 'property' || name === undefined) { return { ok: false, error: { diff --git a/src/util/package-json.ts b/src/util/package-json.ts index 073b36873..ee3d9c956 100644 --- a/src/util/package-json.ts +++ b/src/util/package-json.ts @@ -89,7 +89,7 @@ export class PackageJson { #getOrMakeScriptInfo(name: string): ScriptSyntaxInfo { let info = this.#scripts.get(name); - if (info == null) { + if (info === undefined) { info = {name}; this.#scripts.set(name, info); } @@ -112,11 +112,11 @@ export class PackageJson { return; } const scriptsSection = scriptsSectionResult.value; - if (scriptsSection == null) { + if (scriptsSection === undefined) { return; } const fail = failUnlessJsonObject(scriptsSection, this.jsonFile); - if (fail != null) { + if (fail !== undefined) { failures.push(fail); return; } @@ -124,10 +124,10 @@ export class PackageJson { if (child.type !== 'property') { continue; } - const [rawName, rawValue] = child.children ?? []; - if (rawName == null || rawValue == null) { + if (child.children === undefined) { continue; } + const [rawName, rawValue] = child.children; const nameResult = failUnlessNonBlankString(rawName, this.jsonFile); if (!nameResult.ok) { failures.push(nameResult.error); @@ -167,11 +167,11 @@ export class PackageJson { return; } const wireitSection = wireitSectionResult.value; - if (wireitSection == null) { + if (wireitSection === undefined) { return; } const fail = failUnlessJsonObject(wireitSection, this.jsonFile); - if (fail != null) { + if (fail !== undefined) { failures.push(fail); return; } @@ -179,17 +179,17 @@ export class PackageJson { if (child.type !== 'property') { continue; } - const [rawName, rawValue] = child.children ?? []; - if (rawName == null || rawValue == null) { + if (child.children === undefined) { continue; } + const [rawName, rawValue] = child.children ?? []; const nameResult = failUnlessNonBlankString(rawName, this.jsonFile); if (!nameResult.ok) { failures.push(nameResult.error); continue; } const fail = failUnlessJsonObject(rawValue, this.jsonFile); - if (fail != null) { + if (fail !== undefined) { failures.push(fail); continue; } From a6df3d125e06c56e9bfd201bbea55180d8bdc4e9 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 6 Nov 2022 08:47:22 -0800 Subject: [PATCH 12/14] Add test that service soft dependency does not require restart --- src/test/service.test.ts | 80 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 3753c30e3..582fb26e6 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -960,4 +960,84 @@ test( }) ); +test( + 'service soft dependency does not require restart in watch mode', + timeout(async ({rig}) => { + // service + // / \ + // v v + // hard soft + const service = await rig.newCommand(); + const hard = await rig.newCommand(); + const soft = await rig.newCommand(); + await rig.writeAtomic({ + 'package.json': { + scripts: { + service: 'wireit', + hard: 'wireit', + soft: 'wireit', + }, + wireit: { + service: { + command: service.command, + service: true, + dependencies: [ + 'hard', + { + script: 'soft', + soft: true, + }, + ], + }, + hard: { + command: hard.command, + files: ['input/hard'], + output: [], + }, + soft: { + command: soft.command, + files: ['input/soft'], + output: [], + }, + }, + }, + }); + + // Initial run + await rig.write('input/hard', '1'); + await rig.write('input/soft', '1'); + const wireit = rig.exec('npm run service --watch'); + const hardInv1 = await hard.nextInvocation(); + const softInv1 = await soft.nextInvocation(); + hardInv1.exit(0); + softInv1.exit(0); + const serviceInv1 = await service.nextInvocation(); + await wireit.waitForLog(/Service started/); + await wireit.waitForLog(/Watching for file changes/); + + // Changing input of soft dependency does not restart service + await rig.write('input/soft', '2'); + const softInv2 = await soft.nextInvocation(); + softInv2.exit(0); + await wireit.waitForLog(/Watching for file changes/); + assert.ok(serviceInv1.isRunning); + + // Changing input of hard dependency does restart service + await rig.write('input/hard', '2'); + const hardInv2 = await hard.nextInvocation(); + hardInv2.exit(0); + await serviceInv1.closed; + await service.nextInvocation(); + await wireit.waitForLog(/Service stopped/); + await wireit.waitForLog(/Service started/); + await wireit.waitForLog(/Watching for file changes/); + + wireit.kill(); + await wireit.exit; + assert.equal(service.numInvocations, 2); + assert.equal(hard.numInvocations, 2); + assert.equal(soft.numInvocations, 2); + }) +); + test.run(); From ae18fcb3406e4b292cf2d300132466438940102d Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 6 Nov 2022 11:02:24 -0800 Subject: [PATCH 13/14] Document soft dependencies under services --- README.md | 29 ++++++++++++++++++++++++----- schema.json | 2 +- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6703b36d3..a5b98da96 100644 --- a/README.md +++ b/README.md @@ -480,12 +480,20 @@ expected to exit by itself, set `"service": true`. "command": "node my-server.js", "service": true, "files": ["server-config.json"], - "dependencies": ["build:server", "build:assets"] + "dependencies": [ + "build:server", + { + "script": "build:assets", + "soft": true + } + ] } } } ``` +### Service lifetime + If a service is run _directly_ (e.g. `npm run serve`), then it will stay running until the user kills Wireit (e.g. `Ctrl-C`). @@ -493,13 +501,24 @@ If a service is a _dependency_ of one or more other scripts, then it will start up before any depending script runs, and will shut down after all depending scripts finish. +### Service restarts + In watch mode, a service will be restarted whenever one of its input files or -dependencies change. +dependencies change, except for dependencies annotated as +[`soft`](#soft-dependencies). + +Use `soft` when the output of a dependency is read dynamically for each request +handled by the service. For example, the static assets of a web server can often +be annotated as `soft`. + +### Service output Services cannot have `output` files, because there is no way for Wireit to know -when a service has finished writing its output. If you have a service that -produces output, you should define a non-service script that depends on it, and -which exits when the service's output is complete. +when a service has finished writing its output. + +If you have a service that produces output, you should define a _non-service_ +script that depends on it, and which exits when the service's output is +complete. ## Failures and errors diff --git a/schema.json b/schema.json index 997c14b84..1fe12cf17 100644 --- a/schema.json +++ b/schema.json @@ -35,7 +35,7 @@ "minLength": 1 }, "soft": { - "markdownDescription": "When `false` (the default), whenever this dependency runs, this script (the dependent) will be marked stale and need to re-run too, regardless of whether the dependency produced new or relevant output. When soft is `true` Wireit won't assume that the dependent is stale just because the dependency ran. This can reduce unnecessary re-building when `files` captures all of the relevant output of the dependency .\n\nFor more info, see https://github.com/google/wireit#soft-dependencies", + "markdownDescription": "When `false` (the default), whenever this dependency runs, this script (the dependent) will be marked stale and need to re-run too, regardless of whether the dependency produced new or relevant output. When `false` Wireit won't assume that the dependent is stale just because the dependency ran. This can reduce unnecessary re-building (or restarting in the case of services) when `files` captures all of the relevant output of the dependency.\n\nFor more info, see https://github.com/google/wireit#re-run-on-change", "type": "boolean" } } From 3d155734ab4715ed174ea58c3d6814111af8c588 Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Sun, 6 Nov 2022 11:33:52 -0800 Subject: [PATCH 14/14] Rename soft:true to triggersRerun:false --- CHANGELOG.md | 20 ++++++++-------- README.md | 41 ++++++++++++++++---------------- schema.json | 4 ++-- src/analyzer.ts | 23 +++++++++++------- src/config.ts | 2 +- src/fingerprint.ts | 5 ++-- src/test/basic.test.ts | 6 ++--- src/test/errors-analysis.test.ts | 10 ++++---- src/test/json-schema.test.ts | 12 ++++++---- src/test/service.test.ts | 4 ++-- src/test/util/package-json.ts | 2 +- 11 files changed, 69 insertions(+), 60 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ef077fff..d3d7c1af9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,21 +15,21 @@ Versioning](https://semver.org/spec/v2.0.0.html). or when another script that depends on it is ready to run. A service is stopped when all scripts that depend on it have finished, or when Wireit is exited. -- Added _soft dependencies_. +- Added `"triggersRerun": false` setting to dependencies. By default, the fingerprint of a script includes the fingerprints of its dependencies. This means a script will re-run whenever one of its dependencies re-runs, even if the output produced by the dependency didn't actually change. - Now, if a dependency is annotated with `"soft": true`, then the fingerprint of - that dependency will no longer be included in the script's own fingerprint. - This means a script won't neccessarily re-run just because a dependency re-ran - — though Wireit will still always run the dependency first if it is not - up-to-date. + Now, if a dependency is annotated with `"triggersRerun": false`, then the + fingerprint of that dependency will no longer be included in the script's own + fingerprint. This means a script won't neccessarily re-run just because a + dependency re-ran — though Wireit will still always run the dependency first + if it is not up-to-date. - Using `"soft": true` can result in faster builds thanks to fewer re-runs, but - it is very important to specify all of the input files generated by the - dependency which the script depends on in the `files` array. + Using `"triggersRerun": false` can result in faster builds thanks to fewer + re-runs, but it is very important to specify all of the input files generated + by the dependency which the script depends on in the `files` array. Example: @@ -48,7 +48,7 @@ Versioning](https://semver.org/spec/v2.0.0.html). "dependencies": { [ "script": "build", - "soft": true + "triggersRerun": false ] } } diff --git a/README.md b/README.md index a5b98da96..aa294babd 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,7 @@ - [Dependencies](#dependencies) - [Vanilla scripts](#vanilla-scripts) - [Cross-package dependencies](#cross-package-dependencies) - - [Soft dependencies](#soft-dependencies) + - [Triggers re-run](#triggers-re-run) - [Parallelism](#parallelism) - [Extra arguments](#extra-arguments) - [Input and output files](#input-and-output-files) @@ -169,7 +169,7 @@ workspaces, as well as in other kinds of monorepos. } ``` -### Soft dependencies +### Triggers re-run By default, whenever a dependency runs, the script that depends on it will be marked stale and need to re-run too — regardless of whether the dependency @@ -180,13 +180,13 @@ files for every script when those input files are generated by a dependency. However, it comes with the tradeoff that scripts will sometimes re-run even when none of their input files changed. -To change this behavior and further optimize your build, you can use the `soft` -setting to tell Wireit that all of the input files needed from a dependency are -declared in the `files` array. Now, Wireit won't assume that a script was stale -just because its dependency ran. +To change this behavior and further optimize your build, you can use the +`triggersRerun` setting to tell Wireit that all of the input files needed from +a dependency are declared in the `files` array. Now, Wireit won't assume that a +script was stale just because its dependency ran. -To declare a soft dependency, create an object for your dependency instead of a -plain string, and set `soft` to `true`: +To enable this setting, create an object for your dependency instead of a plain +string, and set `triggersRerun` to `false`: ```json { @@ -195,7 +195,7 @@ plain string, and set `soft` to `true`: "dependencies": [ { "script": "B", - "soft": true + "triggersRerun": false } ] } @@ -203,10 +203,11 @@ plain string, and set `soft` to `true`: } ``` -In the following example, `bundle` has a `soft` dependency on `build`. -Importantly, it also includes `lib/**/*.js` in its `files` array, which are the -specific outputs from `tsc` that `rollup` consumes. Including these input files -wasn't neccessary before, but with `soft` it is now critical. +In the following example, `bundle` has a dependency on `build` with +`triggersRerun: false`. Importantly, it also includes `lib/**/*.js` in its +`files` array, which are the specific outputs from `tsc` that `rollup` consumes. +Including these input files wasn't neccessary before, but with +`triggersRerun: false` it is now critical. ```json { @@ -225,7 +226,7 @@ wasn't neccessary before, but with `soft` it is now critical. "dependencies": [ { "script": "build", - "soft": true + "triggersRerun": false } ], "files": ["rollup.config.json", "lib/**/*.js", "!lib/test"], @@ -484,7 +485,7 @@ expected to exit by itself, set `"service": true`. "build:server", { "script": "build:assets", - "soft": true + "triggersRerun": false } ] } @@ -504,12 +505,12 @@ scripts finish. ### Service restarts In watch mode, a service will be restarted whenever one of its input files or -dependencies change, except for dependencies annotated as -[`soft`](#soft-dependencies). +dependencies change, except for dependencies with +[`triggersRerun: false`](#re-run-on-change). -Use `soft` when the output of a dependency is read dynamically for each request -handled by the service. For example, the static assets of a web server can often -be annotated as `soft`. +Use `triggersRerun: false` when the output of a dependency is read dynamically +for each request handled by the service. For example, the static assets of a web +server can often be annotated with `triggersRerun: false`. ### Service output diff --git a/schema.json b/schema.json index 1fe12cf17..f6fb62c14 100644 --- a/schema.json +++ b/schema.json @@ -34,8 +34,8 @@ "type": "string", "minLength": 1 }, - "soft": { - "markdownDescription": "When `false` (the default), whenever this dependency runs, this script (the dependent) will be marked stale and need to re-run too, regardless of whether the dependency produced new or relevant output. When `false` Wireit won't assume that the dependent is stale just because the dependency ran. This can reduce unnecessary re-building (or restarting in the case of services) when `files` captures all of the relevant output of the dependency.\n\nFor more info, see https://github.com/google/wireit#re-run-on-change", + "triggersRerun": { + "markdownDescription": "When `true` (the default), whenever this dependency runs, this script (the dependent) will be marked stale and need to re-run too, regardless of whether the dependency produced new or relevant output. When `false` Wireit won't assume that the dependent is stale just because the dependency ran. This can reduce unnecessary re-building (or restarting in the case of services) when `files` captures all of the relevant output of the dependency.\n\nFor more info, see https://github.com/google/wireit#re-run-on-change", "type": "boolean" } } diff --git a/src/analyzer.ts b/src/analyzer.ts index d22cd4310..a922b3463 100644 --- a/src/analyzer.ts +++ b/src/analyzer.ts @@ -532,7 +532,7 @@ export class Analyzer { // property plus optional extra annotations. const maybeUnresolved = children[i]; let specifierResult; - let soft = false; // Default; + let triggersRerun = true; // Default; if (maybeUnresolved.type === 'string') { specifierResult = failUnlessNonBlankString( maybeUnresolved, @@ -574,10 +574,15 @@ export class Analyzer { placeholder.failures.push(specifierResult.error); continue; } - const softResult = findNodeAtLocation(maybeUnresolved, ['soft']); - if (softResult !== undefined) { - if (softResult.value === true || softResult.value === false) { - soft = softResult.value; + const triggersRerunResult = findNodeAtLocation(maybeUnresolved, [ + 'triggersRerun', + ]); + if (triggersRerunResult !== undefined) { + if ( + triggersRerunResult.value === true || + triggersRerunResult.value === false + ) { + triggersRerun = triggersRerunResult.value; } else { encounteredError = true; placeholder.failures.push({ @@ -586,12 +591,12 @@ export class Analyzer { script: {packageDir: pathlib.dirname(packageJson.jsonFile.path)}, diagnostic: { severity: 'error', - message: `The "soft" property must be either true or false.`, + message: `The "triggersRerun" property must be either true or false.`, location: { file: packageJson.jsonFile, range: { - offset: softResult.offset, - length: softResult.length, + offset: triggersRerunResult.offset, + length: triggersRerunResult.length, }, }, }, @@ -672,7 +677,7 @@ export class Analyzer { dependencies.push({ specifier: unresolved, config: placeHolderInfo.placeholder, - soft, + triggersRerun, }); this._ongoingWorkPromises.push( (async () => { diff --git a/src/config.ts b/src/config.ts index a5837db5c..8139145e1 100644 --- a/src/config.ts +++ b/src/config.ts @@ -49,7 +49,7 @@ export interface Dependency< > { config: Config; specifier: JsonAstNode; - soft: boolean; + triggersRerun: boolean; } export type ScriptConfig = diff --git a/src/fingerprint.ts b/src/fingerprint.ts index 9305e47b1..49151c438 100644 --- a/src/fingerprint.ts +++ b/src/fingerprint.ts @@ -118,8 +118,9 @@ export class Fingerprint { [ScriptReferenceString, FingerprintData] > = []; for (const [dep, depFingerprint] of dependencyFingerprints) { - if (dep.soft) { - // Soft dependencies aren't included in the fingerprint. + if (!dep.triggersRerun) { + // triggersRerun: false means the fingerprint of the dependency isn't + // directly inherited. continue; } if (!depFingerprint.data.fullyTracked) { diff --git a/src/test/basic.test.ts b/src/test/basic.test.ts index 2333284d0..8399cabdd 100644 --- a/src/test/basic.test.ts +++ b/src/test/basic.test.ts @@ -1044,9 +1044,9 @@ for (const agent of ['npm', 'yarn', 'pnpm']) { } test( - 'soft dependency does not inherit fingerprint', + 'triggersRerun:false dependency does not inherit fingerprint', timeout(async ({rig}) => { - // a --[soft]--> b --> c + // a --[triggersRerun:false]--> b --> c const a = await rig.newCommand(); const b = await rig.newCommand(); const c = await rig.newCommand(); @@ -1063,7 +1063,7 @@ test( dependencies: [ { script: 'b', - soft: true, + triggersRerun: false, }, ], files: ['inputs/a'], diff --git a/src/test/errors-analysis.test.ts b/src/test/errors-analysis.test.ts index 6630407ec..4ce5d0a17 100644 --- a/src/test/errors-analysis.test.ts +++ b/src/test/errors-analysis.test.ts @@ -1954,7 +1954,7 @@ test( ); test( - 'dependencies.soft is not a boolean', + 'dependencies.triggersRerun is not a boolean', timeout(async ({rig}) => { await rig.write({ 'package.json': { @@ -1967,7 +1967,7 @@ test( dependencies: [ { script: 'b', - soft: 1, + triggersRerun: 1, }, ], }, @@ -1983,9 +1983,9 @@ test( checkScriptOutput( done.stderr, ` -❌ package.json:11:19 The "soft" property must be either true or false. - "soft": 1 - ~` +❌ package.json:11:28 The "triggersRerun" property must be either true or false. + "triggersRerun": 1 + ~` ); }) ); diff --git a/src/test/json-schema.test.ts b/src/test/json-schema.test.ts index 8f131e34a..4c1428932 100644 --- a/src/test/json-schema.test.ts +++ b/src/test/json-schema.test.ts @@ -62,8 +62,10 @@ test('dependency object is valid', () => { shouldValidate({wireit: {a: {dependencies: [{script: 'b'}]}}}); }); -test('dependency object with soft annotation is valid', () => { - shouldValidate({wireit: {a: {dependencies: [{script: 'b', soft: true}]}}}); +test('dependency object with triggersRerun:false annotation is valid', () => { + shouldValidate({ + wireit: {a: {dependencies: [{script: 'b', triggersRerun: false}]}}, + }); }); // I couldn't figure out how to make this test pass while keeping the other @@ -80,7 +82,7 @@ test('a script with all fields set is valid', () => { wireit: { a: { command: 'b', - dependencies: ['c', {script: 'c', soft: true}], + dependencies: ['c', {script: 'c', triggersRerun: false}], files: ['d'], output: ['e'], clean: true, @@ -248,13 +250,13 @@ test('dependencies[i].script is required', () => { ); }); -test('dependencies[i].soft must be boolean', () => { +test('dependencies[i].triggersRerun must be boolean', () => { expectValidationErrors( { wireit: { a: { command: 'b', - dependencies: [{script: 'b', soft: 1}], + dependencies: [{script: 'b', triggersRerun: 1}], }, }, }, diff --git a/src/test/service.test.ts b/src/test/service.test.ts index 582fb26e6..516221399 100644 --- a/src/test/service.test.ts +++ b/src/test/service.test.ts @@ -961,7 +961,7 @@ test( ); test( - 'service soft dependency does not require restart in watch mode', + 'service with triggersRerun:false does not require restart in watch mode', timeout(async ({rig}) => { // service // / \ @@ -985,7 +985,7 @@ test( 'hard', { script: 'soft', - soft: true, + triggersRerun: false, }, ], }, diff --git a/src/test/util/package-json.ts b/src/test/util/package-json.ts index 250bb2d7c..efcc1fbd0 100644 --- a/src/test/util/package-json.ts +++ b/src/test/util/package-json.ts @@ -16,7 +16,7 @@ export interface PackageJson { wireit?: { [scriptName: string]: { command?: string; - dependencies?: Array; + dependencies?: Array; files?: string[]; output?: string[]; clean?: boolean | 'if-file-deleted';