FluidFramework icon indicating copy to clipboard operation
FluidFramework copied to clipboard

feat(build-cli): Add per-package typetest entrypoint config

Open tylerbutler opened this issue 1 year ago • 7 comments

Background

The type tests are primarily configured in the typeValidation field in package.json. However, the API level setting is only passed via a CLI flag, so when you make changes to the type tests for a particular project, you have to update the CLI command. It also means you can't use the package filtering flags to operate on a subset of packages with the CLI, which is confusing.

In addition, the type tests always use the index.ts file in the source package to determine what tests to generate, but this does not allow us to test specific API levels and exclude types only exported at certain levels

Changes

The main change is that the tests now use the generated d.ts files to determine what tests to generate, so the generation process now depends on compilation.

There is also a new typeValidation setting, entrypoint, that, when set, will be used as the "default" entrypoint for that package (default value is "legacy"). If the CLI --entrypoint flag is passed it is used instead of the default. (The --level flag is now a deprecated alias of the --entrypoint flag.)

Other changes

  • Updated the root fluid-build task config with the following changes:
    • typetests:gen is now dependent on compilation
    • build:test now depends on typetests:gen
    • commonjs and api no longer depend on typetests:gen
  • Added several projects to the fluid-build-tasks-tsc policy exclusion list. This policy is trying to guard against the issue noted in "concerns" below, but that policy cannot be followed when type tests depend on compilation like they do now.
  • Removed the fluidBuild task overrides for several packages - the ones excluded from the policy - because they were setting the tsc task to depend on typetests:gen. This cannot work when type tests depend on compilation.
  • I pre-applied some of the new settings to package.json files in the client release group. These changes have no effect until the new build-tools is released and integrated, but merging them now will make the build-tools PR smaller. If folks think this should be done separately I will split it out.

Concerns

The task ordering now has a cycle of sorts, because some projects build the type tests as part of their main prod build. Such projects will have a "double build" issue when the type tests change. The first compilation will use the old type test source, then the new type tests will be generated - so the project needs to be built again to pick up those changes.

Addressing this is outside the scope of this PR, but if it's important then we can update the build process in client before we incorporate the new build-tools.

Tips for reviewers

The package.json changes are not very interesting, so you can filter them out: https://github.com/microsoft/FluidFramework/pull/22131/files?file-filters%5B%5D=.cjs&file-filters%5B%5D=.md&file-filters%5B%5D=.ts&show-viewed-files=true

AB#7875

tylerbutler avatar Aug 06 '24 02:08 tylerbutler

@fluid-example/bundle-size-tests: +245 Bytes
Metric NameBaseline SizeCompare SizeSize Diff
aqueduct.js 461.15 KB 461.18 KB +35 Bytes
azureClient.js 559.19 KB 559.24 KB +49 Bytes
connectionState.js 680 Bytes 680 Bytes No change
containerRuntime.js 261.99 KB 262 KB +14 Bytes
fluidFramework.js 400.11 KB 400.12 KB +14 Bytes
loader.js 134.26 KB 134.28 KB +14 Bytes
map.js 42.39 KB 42.39 KB +7 Bytes
matrix.js 146.56 KB 146.56 KB +7 Bytes
odspClient.js 526.47 KB 526.52 KB +49 Bytes
odspDriver.js 97.72 KB 97.74 KB +21 Bytes
odspPrefetchSnapshot.js 42.78 KB 42.79 KB +14 Bytes
sharedString.js 163.26 KB 163.26 KB +7 Bytes
sharedTree.js 390.62 KB 390.63 KB +7 Bytes
Total Size 3.3 MB 3.3 MB +245 Bytes

Baseline commit: 8db660e2d470e3fd590327d646b420893f7041e9

Generated by :no_entry_sign: dangerJS against befd9c58b68b55d4520e4686f5dbc21cb6d0e4d5

msfluid-bot avatar Aug 06 '24 02:08 msfluid-bot

I thought "legacy" was orthogonal to API levels, i.e we (at some point) can have alpha, beta, public, and separately legacy, legacy alpha, legacy beta. So seeing legacy as the default apiLevel feels weird. I think I understand that in practice, today, it's kind of another api level, but if that's the case then I'd like to see a very deliberate acknowledgement of that and design around it.

I don't think API Level is the best term anymore. For the purposes of type tests, these are "export paths" or "subsets of exported APIs with known names that correspond to a known export path." The string here tells us which types to load, and from an implementation perspective it should be able to handle arbitrary strings (e.g. if we had a package with /foo exports, then the tool, if provided the string "foo", would get the types from the /foo entrypoint -- if we allowed arbitrary string input, which we don't. But my point is that the strings are not special from a code perspective.

We also can generate multiple type test files if needed in the future by calling generate typetests multiple times. My original design was to have a type test per export path, but since there's duplication between alpha/beta/public that didn't make much sense in the end. But the infra is there for that to be done in the future if needed.

Maybe renaming the level flag to entrypoint would help clarify? That can be done without a breaking change with a flag alias.

With typetests:gen now depending on build:test, did we check if we're affecting any packages that don't have a separate test build? Off the top of my head I don't recall which ones are like that but I know some exist. Maybe they don't require type tests and we'll be fine, but wanted to call it out.

Because of the way fluid-build works, if a package doesn't have a build:test script, then for that package, build:test is just ignored. In other words, for that package the typetests:gen task would run without waiting for build:test. The change does impact the build though, which I call out in the "concerns" section of the PR description.

tylerbutler avatar Aug 13 '24 20:08 tylerbutler

Maybe renaming the level flag to entrypoint would help clarify? That can be done without a breaking change with a flag alias.

I did this in the latest iteration.

tylerbutler avatar Aug 13 '24 21:08 tylerbutler

With typetests:gen now depending on build:test, did we check if we're affecting any packages that don't have a separate test build? Off the top of my head I don't recall which ones are like that but I know some exist. Maybe they don't require type tests and we'll be fine, but wanted to call it out.

Because of the way fluid-build works, if a package doesn't have a build:test script, then for that package, build:test is just ignored. In other words, for that package the typetests:gen task would run without waiting for build:test. The change does impact the build though, which I call out in the "concerns" section of the PR description.

Correction here: typetests:gen does not depend on build:test - it's the other way around.

tylerbutler avatar Aug 13 '24 23:08 tylerbutler

The main change is that the tests now use the generated d.ts files to determine what tests to generate, so the generation process now depends on compilation.

This seems like an odd choice to me.

Any time the type generation depend on the package's current content is problematic because it's no longer testing that the current package is compatible with the old one.

If a package deletes its beta entry point for example, that's a breaking change, and not something the type tests should automatically account for.

The idea with the type tests is based on the previous version, we generate something that only compiles if the new one is compatible. We don't strictly follow this, but in my opinion we should move toward the type tests being only a function of the config data in the package.json typeValidation block and the previous version of the package.

If I wanted to know what to test, I'd do it by looking at the package.json for the previous version of the package. and testing all its declared entry point d.ts files. This way its testing the API surface the package declares itself as exposing. This would also avoid taking any new dependencies on the current version of the package or its build, and would make removing entry points correctly marked as a breaking change.

If you want to subset that, additional config could be added to the typeValidation block (from the current package). THe simplest way I see to do this is to allow the existing "disabled" entry to be a list of entry points to skip in addition to its current boolean options.

CraigMacomber avatar Aug 14 '24 16:08 CraigMacomber

Marking this as a draft again since #22312 needs to be merged first, after which this PR will change in size and scope.

tylerbutler avatar Aug 26 '24 21:08 tylerbutler

Thanks for the review, @CraigMacomber!

tylerbutler avatar Sep 05 '24 16:09 tylerbutler