Skip to content

Fix bug with phantom transitive dependencies#1031

Open
christiango wants to merge 7 commits intomasterfrom
christiango/isolated-declarations-transitive-deps-bug
Open

Fix bug with phantom transitive dependencies#1031
christiango wants to merge 7 commits intomasterfrom
christiango/isolated-declarations-transitive-deps-bug

Conversation

@christiango
Copy link
Member

This branch fixes a bug where transitive (^^) and topological (^) dependency expansion would pull in "phantom" targets — targets created for packages that don't actually define the referenced script. When those phantom targets are later removed by remove nodes their same-package dependencies get re-wired to the dependents, creating spurious cross-package edges that weren't intended.

The internal repo issue that caused this change has the following behavior we have a repo that is onboarding to typescript isolated declarations to improve build times. With this change we have transpile and typecheck steps. Today typecheck is defined with ^typecheck, meaning all dependencies are typechecked before the current package is typechecked. With isolated declarations we can emit all the d.ts files during transpile and typecheck can instead start when all dependents have produced d.ts. Since isolatedDeclarations is being rolled out slowly in the repo we have the following configuration to support a mix of packages with isolated declarations:

  • a transpile step -> turns ts files into .js. For packages that have isolated declarations enabled, oxc is used to output d.ts files
  • a typecheck step -> consumes all d.ts files of dependencies and checks for type errors. For packages without isolated declarations, this also emits d.ts files
  • an emitDeclarations step -> this is a no-op, but it depends on the packages typecheck. The only purpose of this step is to construct the correct minimal dependency graph for the repo.

The lage config dependencies are:
transpile: [],
emitDeclarations: [typecheck],
typecheck: [^^emitDeclarations, ^^typecheck]

Before this change we were seeing that packages were incorrectly blocking on the typecheck step of any package that uses isolatedDelcarations. Now we do the correc thing

Copy link
Member

@ecraig12345 ecraig12345 left a comment

Choose a reason for hiding this comment

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

As discussed in chat, this needs some additional logic to ensure the target isn't defined in some other way besides as a script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants