FluidFramework
FluidFramework copied to clipboard
fix(build-tools): restore some tsc dep checking
fix(build-tools): restore some tsc dep checking fluid-build-tasks-tsc policy handler was searching for command line of exactly "tsc" which has been mostly abandoned since Node16+ESM pattern.
Create a build database that understands basic build tasks, their output, and their CJS/ESM slant. From that, build approximate predecessor lists to replace "tsc" assumption.
The predecessor list is approximate as a package with multiple ESM (or CJS) build scripts may produce the required output from any one of those scripts. Until tsc incremental is run and generates required inputs, precise dependencies cannot be established. This change does not add a post build policy check for such precise dependencies. So, policy addressee must choose the correct predecessor from a list presented. An example of choices presented:
file failed the "fluid-build-tasks-tsc" policy: packages/test/local-server-tests/package.json
'build:test' task is missing the following dependency:
@fluid-internal/client-utils#build:test:jest or @fluid-internal/client-utils#build:test:mocha:cjs or @fluid-internal/client-utils#tsc
@fluid-internal/mocha-test-setup#tsc
@fluid-private/test-loader-utils#tsc
@fluid-private/test-pairwise-generator#tsc
@fluidframework/aqueduct#api-extractor:commonjs or @fluidframework/aqueduct#build:test:cjs or @fluidframework/aqueduct#tsc
@fluidframework/build-tools#build:test or @fluidframework/build-tools#tsc
@fluidframework/cell#api-extractor:commonjs or @fluidframework/cell#build:test:cjs or @fluidframework/cell#tsc
Refactor logic from entrypoints to be used by database to reason able package exports for generate entrypoints
scripts.
Apply policy corrections. One of note is api-markdown-documenter that builds ESM from script named tsc.
Future: fluid-build-tasks-eslint should be moved off of getDefaultTscTaskDependencies that looks for "tsc".
A few more comments, nothing critical. I can follow the general idea of what's happening and it seems fine, I just can't quite follow the details in my head.
Which part is not as followable. This seems like something that should be sufficiently overviewed/explained somewhere (and not just in PR description).
I'll say that several things in these packages (like
hasTaskDependency
) would benefit from unit tests. I'll leave it up to you if it makes sense to add some here or capture work items for the backlog.
I very much favor having tests for these policies. I generally prefer to avoid whitebox testing but it has its place. Unfortunately, there are no policy tests; so, no infrastructure to add the cases we found are missing. The good news is that we run these against the repo all the time; so, we have a form of testing regularly. Added AB#8007
Which part is not as followable. This seems like something that should be sufficiently overviewed/explained somewhere (and not just in PR description).
It's mostly lack of familiarity with a good chunk of the code this is touching or calling. Having to go several function calls deep in code that's new to me to actually understand what something in your changes is doing, a few times per file or per function, starts overwhelming my capacity to keep a solid mental model of the system as a whole.
I'd say docs on the helper functions (both newly added ones and pre-existing ones you used) would help, that'd be a good place for further explanation so I could either trust the functions and not have to go through them line by line to understand them, or if I decide to still go through them, at least have some baseline of what I'm expecting the code there to be doing.