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

feat: add transitive typecheck output group #714

Merged

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!

@MichaelMitchell-at MichaelMitchell-at force-pushed the transitive_typecheck_output branch from 4ffb628 to dba36f0 Compare October 11, 2024 18:56
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

@MichaelMitchell-at MichaelMitchell-at force-pushed the transitive_typecheck_output branch from dba36f0 to 1bd06dd Compare October 11, 2024 19:01
ts/private/ts_project.bzl Outdated Show resolved Hide resolved
ts/defs.bzl Show resolved Hide resolved
@MichaelMitchell-at MichaelMitchell-at force-pushed the transitive_typecheck_output branch from 1bd06dd to 1706fd5 Compare October 11, 2024 19:23
@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
@MichaelMitchell-at MichaelMitchell-at force-pushed the transitive_typecheck_output branch from 2083d63 to b4166ad Compare October 12, 2024 01:04
@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 🤷

@jbedard jbedard changed the title Add transitive typecheck output group feat: add transitive typecheck output group Dec 20, 2024
@jbedard jbedard merged commit 493e1f1 into aspect-build:main Dec 20, 2024
24 checks passed
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