fix(build-tools): restore some tsc dep checking #20971

jason-ha · 2024-05-03T09:37:15Z

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

Relocate GenerateEntrypointsCommand to library/commands (new folder) so database can use shared logic to get output files. Package exports logic was further broken out and placed under test. Supporting changes:

promote base.ts to library/commands to prevent import cycle
- move rootPathFlag from flags.ts directly into base.ts

Apply policy corrections. One of note is api-markdown-documenter that builds ESM from script named tsc.

Follow-up: fluid-build-tasks-eslint should be moved off of getDefaultTscTaskDependencies that looks for "tsc".

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 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. 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".

tylerbutler

No objection to these changes, but I wonder if they're needed. What do they protect us against? Would it be easier to use fluid-build's task graph for some of the logic? Seems like you're producing the equivalent here so maybe fluid-build itself could be more involved in finding problems?

build-tools/packages/build-cli/src/library/packageExports.ts

tylerbutler · 2024-05-07T18:15:31Z

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts

It's a little unclear to me when I would use the fluidBuildDatabase vs. the task graph from fluid-build.

Task graph is defined before build and very quick. fluidBuildDatabase is imprecise until after the build completes as it only understands output from commands and not the inputs. So, it can say that one of the tasks in package A is needed by B, but not precisely which of those tasks. After a complete build, tsbuildinfo will have complete inputs required including and ESM/CJS cross over. From that input list we can add a [post-build] policy check on task config that is 100% accurate.

After a complete build, tsbuildinfo will have complete inputs required including and ESM/CJS cross over. From that input list we can add a [post-build] policy check on task config that is 100% accurate.

And that input list is what fluidBuildDatabase ends up with, I suppose? Might be worth putting copying this explanation (and a bit more? It's still not 100% clear to me 😅 ) to the class tsdocs.

I mispoke a bit. FluidBuildDatabase doesn't currently end up with that complete information even after build and probably won't. FluidBuildDatabase only has the outputs of scripts. I am adding a comment noting that.
After build we can read the inputs from tsbuildinfo and search FluidBuildDatabase's output knowledge to find source scripts. I don't think we need to store the input into the database, too.

examples/apps/data-object-grid/package.json

tylerbutler · 2024-05-07T18:23:47Z

fluidBuild.config.cjs

+		// Generic build:test script should be replaced by :esm or :cjs specific versions.
+		// "tsc" would be nice to eliminate from here, but plenty of packages still focus
+		// on CommonJS.
+		"build:test": ["typetests:gen", "tsc", "api-extractor:commonjs", "api-extractor:esnext"],


This seems like an odd combination. The test script name isn't specific, so if the package builds both commonjs and esm tests, shouldn't both be built by running build:test?

Suggested change

"build:test": ["typetests:gen", "tsc", "api-extractor:commonjs", "api-extractor:esnext"],

"build:test": ["build:test:cjs", "build:test:esnext"],

If package builds both esm and cjs tests, then those individual test build scripts should have their appropriate dependencies and this isn't needed. This config should be removed, but first the config needs moved to the places that still build tests from build:test. I was doing the minimal changes that are happy with current and new policy and avoiding looking at individual packages.

and this isn't needed

I'd say maybe we still want it? We have the single build/compile that covers commonjs and build:esnext, so having the same pattern for tests makes sense to me. That said, for another PR.

packages/service-clients/end-to-end-tests/azure-client/package.json

tools/getkeys/.eslintrc.cjs

tylerbutler · 2024-05-07T18:30:09Z

tools/api-markdown-documenter/package.json

-			"build:tsc": [
-				"^tsc"
+			"tsc": [
+				"^build:esnext"


Why not rename the script to build:esnext instead of tsc?

Because I was not touching scripts and potentially breaking workflows I didn't know about. Leave that for later/others. tools is outside of the fits a standard pattern world and that is not what this task is about.

I don't see any references to build:tsc in the repo other than the npm script above and the task entry here... not sure what either of them is even accomplishing since nothing else triggers them through fluid build. If we change the task entry I think we want to remove the npm script too. @Josmithr might be able to confirm?

We don't need to remove the script entry even if there was a tie in from something else to build:tsc. build:tsc runs tsc which the fluid build system automatically recognizes as a dependency. So, any build:tsc task in the system has these manually configured dependencies.
If build:tsc script is excessive (unused), then it could be removed.

Yeah, I think it is unused. It's defined and has a task entry here, but no other task/script ever runs it as far as I can tell. Can be removed separately though.

build-tools/packages/build-cli/src/library/packageExports.ts

tools/getkeys/package.json

build-tools/packages/build-cli/src/library/packageExports.ts

Josmithr · 2024-05-09T22:45:50Z

build-tools/packages/build-cli/src/library/packageExports.ts

+		if (typeof value === "string") {
+			if (entry === "types") {
+				for (const [key, apiTagLevel] of mapQueryPathToApiTagLevel.entries()) {
+					// eslint-disable-next-line max-depth


Nit: extracting the body of either for-loop into its own function might make this disable unecessary.

It would. I felt it was better to read the contents of the function here rather than extract for this aggressive rule (my opinion) and try to name/explain the function and handle the return. ?
We can also mitigate rule violation by making line 47 "if" negative and "continue"ing which would bring this all up one level, but continue can be harder to follow.

I'm not particularly concerned about the line length rule itself, more commenting on the fact that there is a lot of logical nesting going on here. I'm wondering if it might make it a bit more readable to pull out pieces and document what they do in isolation. I'm honestly not sure if it would, but there is a fair amount going on here.

I went in to see if comments would help clear things up and that lead me to the things that aren't handled as we don't author outside of a pattern. I started editing to handle those and found a way to move a little out and I think it is better now.

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts

alexvy86 · 2024-05-09T23:07:46Z

packages/test/local-server-tests/package.json

+	"fluidBuild": {
+		"tasks": {
+			"build:test": [
+				"^tsc",


For my/reviewer reference, packages have build:test depend on ^tsc + ^api-extractor:commonjs when they are commonjs and on ^build:esnext + ^api-extractor:esnext when they're esm.

Tangent: we should probably make this package esm, but not in this PR.

Yep. And we prefer to have build:test:cjs|esm for clarity. build:test can exist to run either or both of those but doesn't need dependencies itself. We just need to make sure there are no more build:test that do things directly.

Re Tangent: I knew about this. For now, I thought it was good to have something that runs CommonJS. There are some tasks to make sure we have correct coverage on ESM / CJS.

packages/test/test-end-to-end-tests/package.json

alexvy86

Haven't looked at the fluidBuildDatabase or the policy nor fluidBuildTasks.ts; the rest looks ok except for potentially one comment left independently above.

alexvy86

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.

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.

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts

build-tools/packages/build-cli/src/library/packageExports.ts

alexvy86 · 2024-05-10T19:29:24Z

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.

Josmithr · 2024-05-10T20:57:56Z

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts

+	pkg: Package,
+	commandLine: string,
+): { files: AbsoluteFilePath[]; type: ModuleType | undefined } | undefined {
+	if (!commandLine.startsWith("flub generate entrypoints")) {


Is this check because this function runs as a part of flub generate entrypoints? This feels fragile to me. At the very least, it seems like we should extract this into a constant with some docs.

This is not a part of flub generate entrypoints. This is helper to derive what output a flub command would generate. generate entrypoints is the only one I know we need.
It would be better if *Task classes or some part of them could be leveraged. I checked that out briefly but there are too many dependencies and complications.
Same for the fluid commands. Maybe if I understood oclif better there would be a better refactor to reuse the command processing. There is also a complication with code organization and cyclical import dependencies that make relocating Commands hard. Best I could do was extract some common logic. Having said that I might actually be able to clear up the cycle and do something better.

Relocated command and shared more logic

- tighten up typing with readonly - some refactoring for readability - revise isTypeOnly handling to support more exports structuring patterns - add unit tests for packageExports.ts

to library under new commands folder for logic reuse - promote base.ts to library to prevent import cycle - move rootPathFlag from flags.ts

to get outputs

per [ts-node issue783](TypeStrong/ts-node#783)

as only use is within library

build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts

alexvy86 · 2024-05-13T16:18:38Z

build-tools/packages/build-cli/src/library/commands/base.ts

+const rootPathFlag = Flags.custom({
+	description: "Root directory of the Fluid repo (default: env _FLUID_ROOT_).",
+	env: "_FLUID_ROOT_",
+	hidden: true,
+});


I was already writing that this felt better with the rest of the reusable flags but then noticed it's part of the BaseCommand that every other one extends and it's not referenced anywhere else, so I think moving it here makes sense.

build-tools/packages/build-cli/src/library/commands/constants.ts

alexvy86 · 2024-05-13T16:32:40Z

build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts

+/**
+ * Generates type declarations files for Fluid Framework APIs to support API levels (/alpha, /beta. etc.).
+ */
+export class GenerateEntrypointsCommand extends BaseCommand<


I find it weird to move the whole command (and BaseCommand) to library/ instead of just the necessary helper functions (cursory inspection seemed to suggest we can move just some functions but maybe I missed something). Wouldn't block on this but seems worth revisiting. If it's absolutely necessary, so be it.

Moving just some helpers is what I originally was doing. The real thing that we want to share is the argument processing up to the point of knowing what files are generated. We can't share the argument processing as it is too opaque (at least I couldn't find the way to extract that). So, the next best is to have very similar and somewhat tied together processing in one file and for that I needed the flags out of Command. It didn't seem readable to separate flags from the Command. So, now they can be in one place and we can access two parts as needed.
Another option was to get repoPolicyCheck out of library, but that appeared more complicated.

alexvy86 · 2024-05-13T16:44:36Z

build-tools/packages/build-cli/test/library/packageExports.test.ts

+	actual: Map<TKeys, TValues>,
+	expected: Map<TKeys, TValues>,
+): void {
+	assert.hasAllKeys(actual, [...expected.keys()]);


What scenario does assert.hasAllKeys cover that isn't handled by the loop below?

assert.hasAllKeys confirms that keys are identical - those keys and only those keys (I think the name could be better). The below would allow extra keys.

You could simplify all of this as something like

assert.deepEquals([...actual.entries()], [...expected.entries()]);

I don't know if the error messages would be as useful though.

I liked the output of hasAllKeys. And then there is some nicety to being able to break/debug on single value. Thanks though :)

build-tools/packages/build-cli/src/library/packageExports.ts

build-tools/packages/build-cli/test/library/packageExports.test.ts

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts

Josmithr

My overall understanding of how fluid-build functions is pretty limited. But given that, these changes seem reasonable to me.

I would really love to see us invest in moving away from fluid-build more generally, though. The logic involved keeps expanding, and the set of people who know what's going on is very small.

github-actions bot added area: build Build related issues area: examples Changes that focus on our examples area: tests Tests to add, test infrastructure improvements, etc base: main PRs targeted against main branch labels May 3, 2024

jason-ha force-pushed the restore-approximate-build-dependencies branch from fc63ea3 to 893747d Compare May 3, 2024 17:20

jason-ha marked this pull request as ready for review May 3, 2024 17:33

jason-ha requested review from tylerbutler, alexvy86 and curtisman May 3, 2024 17:33

tylerbutler reviewed May 7, 2024

View reviewed changes

merge: 'main' into restore-approximate-build-dependencies

d1cee7d

jason-ha requested review from tylerbutler and Josmithr May 9, 2024 07:11

Josmithr reviewed May 9, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/packageExports.ts Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/packageExports.ts Outdated Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

tools/getkeys/package.json Outdated Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/packageExports.ts Outdated Show resolved Hide resolved

Josmithr reviewed May 9, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts Show resolved Hide resolved

alexvy86 reviewed May 9, 2024

View reviewed changes

packages/test/test-end-to-end-tests/package.json Show resolved Hide resolved

alexvy86 reviewed May 9, 2024

View reviewed changes

jason-ha added 2 commits May 9, 2024 22:35

tools(tools): remove unused tsc entry

b74eda1

docs(build-tools): comments

684d83f

jason-ha requested review from alexvy86 and Josmithr May 10, 2024 05:43

jason-ha added 2 commits May 9, 2024 23:13

improvement(build-tools): make query function out data agnostic

4167063

merge: 'main' into restore-approximate-build-dependencies

8feea1a

alexvy86 approved these changes May 10, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts Outdated Show resolved Hide resolved

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts Outdated Show resolved Hide resolved

merge: 'main' into restore-approximate-build-dependencies

557cc8f

Josmithr reviewed May 10, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/packageExports.ts Outdated Show resolved Hide resolved

Josmithr reviewed May 10, 2024

View reviewed changes

jason-ha added 10 commits May 10, 2024 18:41

improvement(build-tools): package export proccessing

4ad17cd

- tighten up typing with readonly - some refactoring for readability - revise isTypeOnly handling to support more exports structuring patterns - add unit tests for packageExports.ts

merge: 'main' into restore-approximate-build-dependencies

c770cad

improvement(build-tools): lint and doc updates

6010383

improvement(build-tools): tiny tweaks

4d18956

docs(build-tools): maintenance note

370c67b

style(build-tools): function rename

9bb3c4b

refactor(build-tools): relocate GenerateEntrypoints

782a894

to library under new commands folder for logic reuse - promote base.ts to library to prevent import cycle - move rootPathFlag from flags.ts

refactor(build-tools): use command logic

fafdfce

to get outputs

refactor(build-tools): dedupe import from refactor

eb97db0

test(build-tools): strip .js from imports

4a34749

per [ts-node issue783](TypeStrong/ts-node#783)

jason-ha requested a review from Josmithr May 12, 2024 17:22

improvement(client): remove packageExports exports from library

6d71241

as only use is within library

Josmithr reviewed May 13, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts Show resolved Hide resolved

Josmithr reviewed May 13, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts Outdated Show resolved Hide resolved

docs(build-tools): added comments

b3074d1

alexvy86 reviewed May 13, 2024

View reviewed changes

style(build-tools): rename and doc suggestions

64a2957

Josmithr reviewed May 13, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildDatabase.ts Outdated Show resolved Hide resolved

Josmithr reviewed May 13, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts Show resolved Hide resolved

Josmithr reviewed May 13, 2024

View reviewed changes

build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts Outdated Show resolved Hide resolved

Josmithr approved these changes May 13, 2024

View reviewed changes

jason-ha added 2 commits May 13, 2024 17:34

improvement(build-tools): comments and readonly updates

1a6089a

merge: 'main' into restore-approximate-build-dependencies

4c02272

jason-ha enabled auto-merge (squash) May 14, 2024 00:37

jason-ha merged commit 05b3ebc into microsoft:main May 14, 2024
46 checks passed

jason-ha deleted the restore-approximate-build-dependencies branch May 14, 2024 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(build-tools): restore some tsc dep checking #20971

fix(build-tools): restore some tsc dep checking #20971

jason-ha commented May 3, 2024 •

edited

tylerbutler left a comment

tylerbutler May 7, 2024

jason-ha May 8, 2024

alexvy86 May 9, 2024

jason-ha May 10, 2024

tylerbutler May 7, 2024

jason-ha May 8, 2024

alexvy86 May 9, 2024 •

edited

tylerbutler May 7, 2024

jason-ha May 8, 2024

alexvy86 May 9, 2024

jason-ha May 9, 2024

alexvy86 May 10, 2024

Josmithr May 9, 2024 •

edited

jason-ha May 9, 2024

Josmithr May 10, 2024

jason-ha May 12, 2024

alexvy86 May 9, 2024

jason-ha May 9, 2024

alexvy86 left a comment

alexvy86 left a comment •

edited

alexvy86 commented May 10, 2024

Josmithr May 10, 2024

jason-ha May 12, 2024

jason-ha May 12, 2024

alexvy86 May 13, 2024

alexvy86 May 13, 2024

jason-ha May 13, 2024

alexvy86 May 13, 2024

jason-ha May 13, 2024

Josmithr May 13, 2024 •

edited

jason-ha May 14, 2024

Josmithr left a comment

	"build:test": ["typetests:gen", "tsc", "api-extractor:commonjs", "api-extractor:esnext"],
	"build:test": ["build:test:cjs", "build:test:esnext"],

fix(build-tools): restore some tsc dep checking #20971

fix(build-tools): restore some tsc dep checking #20971

Conversation

jason-ha commented May 3, 2024 • edited

tylerbutler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvy86 May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr May 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvy86 left a comment

Choose a reason for hiding this comment

alexvy86 left a comment • edited

Choose a reason for hiding this comment

alexvy86 commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Josmithr left a comment

Choose a reason for hiding this comment

jason-ha commented May 3, 2024 •

edited

alexvy86 May 9, 2024 •

edited

Josmithr May 9, 2024 •

edited

alexvy86 left a comment •

edited

Josmithr May 13, 2024 •

edited