Skip to content

Services: Add service-related analysis properties - #466

Merged
aomarks merged 6 commits into
servicefrom
service.3
Oct 19, 2022
Merged

Services: Add service-related analysis properties#466
aomarks merged 6 commits into
servicefrom
service.3

Conversation

@aomarks

@aomarks aomarks commented Oct 19, 2022

Copy link
Copy Markdown
Member

Adds 3 properties to analysis that are useful for services:

  1. isDirectlyInvoked tells us whether a service is either the root script, or there's a path from the root script to the service that only passes through no-command scripts (which counts the same way). This is useful because being directly invoked means you should only exit when wireit exits; instead of when all scripts that depend on you have finished.

  2. services lists the services that must be started before a script can start. This isn't quite as simple as "my dependencies which are services", again because of needing to traverse through no-command scripts.

  3. serviceConsumers is the reverse of services. It's useful for services to know which scripts could potentially depend on a script, because once we start a service, we don't want to shut it down until we know that every depending script has either finished, or didn't need to run at all. Knowing how many to expect from the start makes this simpler.

Also adds a unit test that invokes the Analyzer directly. All of our other tests are integration, but in this case it's useful to be able to check the analysis result directly.

Also noticed our uvu test patterns were effectively suffix matches, instead of exact matches (I noticed because analysis\.test\.js$ was accidentally matching both analysis.test.js and errors-analysis.test.js).

Part of #33

Comment thread src/analyzer.ts
private _checkForCyclesAndSortDependencies(
config: LocallyValidScriptConfig | ScriptConfig | InvalidScriptConfig,
trail: Set<ScriptReferenceString>
trail: Set<ScriptReferenceString>,

Copy link
Copy Markdown

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

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

Copy link
Copy Markdown

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.

Comment thread src/analyzer.ts
Comment thread src/config.ts Outdated
* Which scripts depend on this service.
*
* "Effective" meaning these are not necessarily our direct dependents, since
* we include transitive service dependencies through no-command scripts.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Can you comment on which order these are in since you use "reversed"? I presume this is in deepest dependencies first? And do you need to store it reversed? Can you reverse the array at the use site?

@aomarks aomarks Oct 19, 2022

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.

The order in the array doesn't matter. This is reverse in the sense of the direction of the edges in the build graph; it's the opposite direction of effectiveServiceDependencies.

  • effectiveServiceDependencies: The services I depend on
  • reverseEffectiveServiceDependencies: The scripts that depend on me

I thought about using dependency vs dependent to distinguish, but I find that it's really easy to misread those.

Any other suggestions for the names?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

ohhh! Sorry. Yeah, reverse is a little confusing to me. dependent might be better even if it's visually close because it's semantically clearer. I would probably default to that unless something clearly better is found. Maybe something with "back" like a back reference would work?

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.

Renamed to services and serviceConsumers

@aomarks
aomarks requested a review from justinfagnani October 19, 2022 17:21
@aomarks
aomarks merged commit 35d0980 into service Oct 19, 2022
@aomarks
aomarks deleted the service.3 branch October 19, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants