Skip to content
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.

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

Add transitive typecheck output group #714

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

MichaelMitchell-at
Copy link
Contributor

@MichaelMitchell-at MichaelMitchell-at commented Oct 11, 2024

Changes are visible to end-users: yes/no

With isolated declarations in TypeScript, it's no longer necessarily the case that typechecking a project also causes all of its dependencies to be typechecked. Some people prefer/expect the previous behavior when running typechecking locally, so this PR exposes a test target that will always typecheck a project and its dependencies, regardless whether any of them have enabled the isolated_typecheck flag.

Test plan

Works well in our codebase!

Copy link

aspect-workflows bot commented Oct 11, 2024

Test

All tests were cache hits

139 tests (100.0%) were fully cached saving 10s.


Buildifier      Format

ts/private/ts_project.bzl Outdated Show resolved Hide resolved
ts/defs.bzl Show resolved Hide resolved
@jbedard
Copy link
Member

jbedard commented Oct 11, 2024

Can you explain the use case again in the PR description?

@MichaelMitchell-at
Copy link
Contributor Author

Can you explain the use case again in the PR description?

Done!

ts/defs.bzl Outdated Show resolved Hide resolved
ts/defs.bzl Outdated Show resolved Hide resolved
@gregmagolan
Copy link
Member

This is cool. Needs to be more discoverable. There are some docs on macro expansion in docs/transpiler.md that should be updated if a new expansion is added.

I'll pull it down and experiment with it over the weekend. Feels like it should be possible to set as the default behavior as long as the type check actions are still able to be run in parallel.

@jbedard
Copy link
Member

jbedard commented Oct 12, 2024

@gregmagolan I was thinking of explicitly not making it discoverable to start, and marking it as manual so it doesn't show up doing test //.... Mainly just so we can try it out before making it public API. Even just so people can test it at HEAD for now 🤷

I think you are correct that the type-check actions should still be able to run in parallel, we just need to make sure we set this _test target is setup correctly and make sure. In that case I think we can do that by default and we only need one build_test target? Which would be another reason not to make this new one public to start...?

@jbedard
Copy link
Member

jbedard commented Oct 12, 2024

For the parallelization, assuming we have isolated_typecheck enabled:

  • each _typecheck (slow tsc) target needs the _types of each dependency
  • _types can all be done in parallel by the declaration_transpiler
  • nothing depends on _typecheck, so tsc never depends on another tsc

For this _transitive_typecheck... that output_group is basically just a depset of tsc actions, but those actions themselves never depend on eachother so I think they will still all be done in parallel? So do we still need the non-transitive one? I feel like "typecheck this target only" should still be an option but 🤷

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.

3 participants