Skip to content

Services: Add service-related analysis properties #466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aomarks merged 6 commits into from
Oct 19, 2022
Merged

Services: Add service-related analysis properties #466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f898813
Add isDirectlyInvoked to ServiceScriptConfigs
aomarks Oct 19, 2022
fc8e9db
Add effectiveServiceDependencies property
aomarks Oct 19, 2022
cb0adac
Add reverseEffectiveServiceDependencies
aomarks Oct 19, 2022
7fc8e11
Add direct analysis unit tests
aomarks Oct 19, 2022
40aa359
Make uvu test patterns more strict
aomarks Oct 19, 2022
8697b30
Rename service dependency/dependent properties
aomarks Oct 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 30 additions & 20 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
"format:check": "prettier . -c",
"test": "wireit",
"test:headless": "wireit",
"test:analysis": "wireit",
"test:basic": "wireit",
"test:cache-github": "wireit",
"test:cache-local": "wireit",
Expand Down Expand Up @@ -71,6 +72,7 @@
},
"test:headless": {
"dependencies": [
"test:analysis",
"test:basic",
"test:cache-github",
"test:cache-local",
Expand Down Expand Up @@ -107,128 +109,136 @@
],
"output": []
},
"test:analysis": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^analysis\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:basic": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"basic\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^basic\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:cache-github": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cache-github\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cache-github\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:cache-local": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cache-local\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cache-local\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:clean": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"clean\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^clean\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:cli-options": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"cli-options\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^cli-options\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:codeactions": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"codeactions\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^codeactions\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:copy": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"copy\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^copy\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:delete": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"delete\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^delete\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:diagnostic": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"diagnostic\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^diagnostic\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:errors-analysis": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"errors-analysis\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^errors-analysis\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:errors-usage": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"errors-usage\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^errors-usage\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:failures": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"failures\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^failures\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:freshness": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"freshness\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^freshness\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:glob": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"glob\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^glob\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:ide": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"ide\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^ide\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:json-schema": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"json-schema\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^json-schema\\.test\\.js$\"",
"dependencies": [
"build"
],
Expand All @@ -238,31 +248,31 @@
"output": []
},
"test:optimize-mkdirs": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"optimize-mkdirs\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^optimize-mkdirs\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:parallelism": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"parallelism\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^parallelism\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:service": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"service\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^service\\.test\\.js$\"",
"dependencies": [
"build"
],
"files": [],
"output": []
},
"test:watch": {
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"watch\\.test\\.js$\"",
"command": "cross-env NODE_OPTIONS=--enable-source-maps uvu lib/test \"^watch\\.test\\.js$\"",
"dependencies": [
"build"
],
Expand Down
60 changes: 51 additions & 9 deletions src/analyzer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ export class Analyzer {
}
// We don't care about the result, if there's a cycle error it'll
// be added to the scripts' diagnostics.
this._checkForCyclesAndSortDependencies(info.placeholder, new Set());
this._checkForCyclesAndSortDependencies(
info.placeholder,
new Set(),
true
);
}

return this._getDiagnostics();
Expand Down Expand Up @@ -196,7 +200,8 @@ export class Analyzer {
}
const cycleResult = this._checkForCyclesAndSortDependencies(
rootConfig,
new Set()
new Set(),
true
);
if (!cycleResult.ok) {
return {
Expand Down Expand Up @@ -488,6 +493,7 @@ export class Analyzer {
scriptAstNode: scriptCommand,
configAstNode: wireitConfig,
declaringFile: packageJson.jsonFile,
services: [],
};
Object.assign(placeholder, remainingConfig);
}
Expand Down Expand Up @@ -939,7 +945,8 @@ export class Analyzer {
*/
private _checkForCyclesAndSortDependencies(
config: LocallyValidScriptConfig | ScriptConfig | InvalidScriptConfig,
trail: Set<ScriptReferenceString>
trail: Set<ScriptReferenceString>,

@justinfagnani justinfagnani Oct 19, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Might be a nicer callsite if you gave trail and isDirectlyInvoked default values, like trail: Set<ScriptReferenceString> = new Set(). Especially trail, but it also looks like isDirectlyInvoked is only potentially false when it's called recursively.

@aomarks aomarks Oct 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually prefer not having a default, because it forces you to think about what the value should be at every call site. It might be easy to use the default by mistake, otherwise, because you forgot the parameter was there at all.

@justinfagnani justinfagnani Oct 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree sometimes. In recursive calls where you need the initial empty value I often prefer to not require the outer call to provide that value, since it usually can only be one valid thing... to the point where a lot of times I would recommend two functions with the public one not having the parameter at all so that the outer caller can't call the function with an incorrect value. I find default parameters a nice middle ground though.

isDirectlyInvoked: boolean
): Result<ScriptConfig, InvalidScriptConfig> {
if (config.state === 'valid') {
// Already validated.
Expand Down Expand Up @@ -1057,16 +1064,36 @@ export class Analyzer {
dependencyStillUnvalidated = dependency.config;
continue;
}
const result = this._checkForCyclesAndSortDependencies(
dependency.config,
trail
);
if (!result.ok) {
const validDependencyConfigResult =
this._checkForCyclesAndSortDependencies(
dependency.config,
trail,
// Walk through no-command scripts when determining if something is
// being directly invoked (e.g. if the top-level script has no command
// and simply delegates to one or more other scripts, then those
// dependencies are effectively being directly invoked).
isDirectlyInvoked && config.command === undefined
);
if (!validDependencyConfigResult.ok) {
return {
ok: false,
error: this._markAsInvalid(config, result.error.dependencyFailure),
error: this._markAsInvalid(
config,
validDependencyConfigResult.error.dependencyFailure
),
};
}
const validDependencyConfig = validDependencyConfigResult.value;
if (validDependencyConfig.service) {
// We directly depend on a service.
config.services.push(validDependencyConfig);
} else if (validDependencyConfig.command === undefined) {
Comment thread
justinfagnani marked this conversation as resolved.
// We depend on a no-command script, so in effect we depend on all of
// the services it depends on.
for (const service of validDependencyConfig.services) {
config.services.push(service);
}
}
}
trail.delete(trailKey);
}
Expand Down Expand Up @@ -1101,6 +1128,8 @@ export class Analyzer {
// Unfortunately TypeScript doesn't narrow the ...config spread, so we
// have to assign explicitly.
command: config.command,
isDirectlyInvoked,
serviceConsumers: [],
};
} else {
validConfig = {
Expand All @@ -1114,6 +1143,19 @@ export class Analyzer {
};
}

// Propagate reverse service dependencies.
if (validConfig.command) {
for (const dependency of validConfig.dependencies) {
if (dependency.config.service) {
dependency.config.serviceConsumers.push(validConfig);
} else if (dependency.config.command === undefined) {
for (const service of dependency.config.services) {
service.serviceConsumers.push(validConfig);
}
}
}
}

// We want to keep the original reference, but get type checking that
// the only difference between a ScriptConfig and a
// LocallyValidScriptConfig is that the state is 'valid' and the
Expand Down
15 changes: 15 additions & 0 deletions src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ export interface ServiceScriptConfig
extends BaseScriptConfig,
ScriptReferenceWithCommand {
service: true;

/**
* Whether this service is being invoked directly (e.g. `npm run serve`).
*/
isDirectlyInvoked: boolean;

/**
* Scripts that depend on this service.
*/
serviceConsumers: Array<ServiceScriptConfig | StandardScriptConfig>;
}

/**
Expand All @@ -97,6 +107,11 @@ interface BaseScriptConfig extends ScriptReference {
*/
dependencies: Array<Dependency<ScriptConfig>>;

/**
* The services that need to be started before we can run.
*/
services: Array<ServiceScriptConfig>;

/**
* Input file globs for this script.
*
Expand Down
Loading