Skip to content

Reduce dependencies of linting #25895

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

Reduce dependencies of linting #25895

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
853320d
Reduce dependencies of linting
CraigMacomber Nov 18, 2025
b632d52
Fix config to include generated code, and skip warning
CraigMacomber Nov 19, 2025
fc14568
Remove disabled check
CraigMacomber Nov 19, 2025
55f7390
More comments
CraigMacomber Nov 19, 2025
410368b
Merge branch 'main' of https://github.com/microsoft/FluidFramework in…
CraigMacomber Nov 19, 2025
f83cf85
misc task cleanup
CraigMacomber Nov 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions fluidBuild.config.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ module.exports = {
"build:test": ["typetests:gen", "tsc"],
"build:test:cjs": ["typetests:gen", "tsc"],
"build:test:esm": ["typetests:gen", "build:esnext"],
// Everything needed for the API of the package to be consumable by its dependencies.
"api": {
dependsOn: ["api-extractor:commonjs", "api-extractor:esnext"],
script: false,
Expand Down Expand Up @@ -129,9 +130,10 @@ module.exports = {
},
"check:biome": [],
"check:prettier": [],
// ADO #7297: Review why the direct dependency on 'build:esm:test' is necessary.
// Should 'compile' be enough? compile -> build:test -> build:test:esm
"eslint": ["compile", "build:test:esm"],
// 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"],
Copy link
Contributor

@jason-ha jason-ha Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@CraigMacomber CraigMacomber Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@jason-ha jason-ha Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jason-ha jason-ha Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use ^api-extractor:esnext" generally instead of ^api` that will require both ESM and CommonJS builds of all dependencies.

Copy link
Contributor Author

@CraigMacomber CraigMacomber Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jason-ha jason-ha Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"good-fences": [],
"format:biome": [],
"format:prettier": [],
Expand Down
4 changes: 4 additions & 0 deletions packages/dds/tree/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,10 @@
"...",
"@fluidframework/id-compressor#build:test:esm"
],
"eslint": [
"...",
"@fluidframework/id-compressor#build:test:esm"
Copy link
Contributor Author

@CraigMacomber CraigMacomber Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not currently required, but does not harm and would be required if we removed the workaround where we build esm:tests

Copy link
Contributor

@jason-ha jason-ha Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

],
"ci:build:docs": [
"build:esnext"
]
Expand Down
Loading