Reduce dependencies of linting #25895
Conversation
github-actions
bot
added
area: dds
| ], | ||
| "eslint": [ | ||
| "...", | ||
| "@fluidframework/id-compressor#build:test:esm" |
There was a problem hiding this comment.
this is not currently required, but does not harm and would be required if we removed the workaround where we build esm:tests
There was a problem hiding this comment.
Why does tree require its tests to be built before linting? I would expect everything leading up to building of tests would be required, but not that the tests themselves are built.
|
So apparently fixing this violates our policy check because we validate that the dependencies I removed exist.
It sems like we have logic to determine what eslint is supposed to depend on programmatically: https://github.com/microsoft/FluidFramework/blob/main/build-tools/packages/build-cli/src/library/repoPolicyCheck/fluidBuildTasks.ts#L189 And then we also have a specification for what it depends on in the fluid-build config. Having both those seems like a bad idea, and we can't improve this config unless we also fix that other place, then release build tools and integrate it into client. |
fluidBuild.config.cjs
Outdated
| // Linting can require resolving imports into dependencies, and thus "^api". | ||
| // The lint task does not seem to invalidate properly, so we take a dependency on "build:test:esm" as well so lint will rerun when the code changes. | ||
| // This may be related to AB#7297. | ||
| "eslint": ["^api", "build:test:esm"], |
There was a problem hiding this comment.
If a package does not have build:test:esm, might it fail to ensure prereqs are handled?
I agree that compile was too aggressive. Really want everything leading up to compile without actual compile.
There was a problem hiding this comment.
"build:test:esm" is not about prerequisites. It is actually fully unnecessary when linting from a clean state and removing it would greatly improve tree's build times (12 seconds for just that package). It is only there to work around an incremental build bug where lint won't run when you modify source code unless we include this.
So packages without that target might fail to lint incrementally. That's not a huge risk, as CI will catch violations with its clean build.
What we really should do is fix linting so that it actually gets invalidated correctly without this workaround.
There was a problem hiding this comment.
I understand. While build:test:esm may be proxy for when to build, it also ensures that all of its pre-reqs are built which may be pre-reqs for linting.
fluidBuild.config.cjs
Outdated
| // Linting can require resolving imports into dependencies, and thus "^api". | ||
| // The lint task does not seem to invalidate properly, so we take a dependency on "build:test:esm" as well so lint will rerun when the code changes. | ||
| // This may be related to AB#7297. | ||
| "eslint": ["^api", "build:test:esm"], |
There was a problem hiding this comment.
Can this use ^api-extractor:esnext" generally instead of ^api` that will require both ESM and CommonJS builds of all dependencies.
There was a problem hiding this comment.
I believe it can, if all our packages lint esm builds. Some currently do not, like experimental/PropertyDDS/packages/property-properties . That package is one of several though that seems to have some issues with this change and likely have other undeclared dependencies.
There was a problem hiding this comment.
The one-off packages can be handled by setting their own local build config to override the general one that we'd like to apply broadly.
|
Look like it was too eager, and didn't validate enough when making this change. Several packages are failing on clean lints and need investigations and mitigations. |
3 participants
Description
Avoid doing API-reports before eslint.
In my local testing thins speeds up incremental builds of shared tree by ~10 seconds (from 54 to 44 seconds). Clean builds of just the tree package should improve a little more than that.
This leaves another roughly 13 seconds of improvement not gained by due to the delays from building the esm tests as a workaround for the incremental build bug.
Reviewer Guidance
The review process is outlined on this wiki page.