FluidFramework
FluidFramework copied to clipboard
feat(build-cli): Add per-package typetest entrypoint config
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:genis now dependent on compilationbuild:testnow depends ontypetests:gencommonjsandapino longer depend ontypetests: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
tsctask to depend ontypetests: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
⯅ @fluid-example/bundle-size-tests: +245 Bytes
| Metric Name | Baseline Size | Compare Size | Size 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
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.
Maybe renaming the
levelflag toentrypointwould help clarify? That can be done without a breaking change with a flag alias.
I did this in the latest iteration.
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.
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.
Marking this as a draft again since #22312 needs to be merged first, after which this PR will change in size and scope.
Thanks for the review, @CraigMacomber!