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

[Bug]: Superfluous @ts_project_typecheck_test target being generated with v3.2.0 #719

Open
fhanau opened this issue Oct 18, 2024 · 4 comments · May be fixed by #721
Open

[Bug]: Superfluous @ts_project_typecheck_test target being generated with v3.2.0 #719

fhanau opened this issue Oct 18, 2024 · 4 comments · May be fixed by #721
Assignees
Labels
bug Something isn't working

Comments

@fhanau
Copy link

fhanau commented Oct 18, 2024

What happened?

When upgrading from rules_ts v3.1.0 => 3.2.0 or higher, a new @ts_project_typecheck_test test target is unexpectedly being generated for some ts_project-based targets. I believe that this is being caused by an unintentional logic change in #705 that causes the target to be newly generated when the declaration parameter is set to false. https://github.com/aspect-build/rules_ts/pull/705/files#diff-fd5b55e792f846b8312680267efd413bf73bfe655630485e1514db77eb7d187cR399

My understanding is that this target is not needed since tsc will still be invoked as long as no custom transpiler is set and js files are still being generated. The test target fails in some configurations for our build (including the Windows build), but we can't rule out that this is due to an unrelated issue with our build. The issue here is that an unnecessary additional target is being generated.

Version

Development (host) and target OS/architectures:
macOS Sonoma 14.7 (among others)

Output of bazel --version:
bazel 7.3.1
Version of the Aspect rules, or other relevant rules from your WORKSPACE or MODULE.bazel file:
rules_js v2.0.1
bazel-lib 2.9.2

Language(s) and/or frameworks involved:
TypeScript

How to reproduce

Declare a dummy ts_project target in a bazel file:
ts_project(
    name = "dummy@ts_project",
    srcs = ["dummy.ts"],
    tsconfig = "tsconfig.json",
)
alongside an empty dummy.ts file and a tsconfig.json file with no content other than `{}`. Then rules_ts will generate a superfluous dummy@ts_project_typecheck_test target starting with v3.2.0 that was not generated before.

Any other information?

No response

@fhanau fhanau added the bug Something isn't working label Oct 18, 2024
jbedard added a commit to jbedard/rules_ts that referenced this issue Oct 18, 2024
@jbedard
Copy link
Member

jbedard commented Oct 18, 2024

Is this actually causing any issues? Or just seems like extra spam?

I've been debating declaring these targets 100% of the time, just like the _types target is now always exposed. That way the rule consistently outputs the same labels and the user invoking or depending on a target doesn't need to know if a transpiler is being used etc.

@fhanau
Copy link
Author

fhanau commented Oct 19, 2024

Is this actually causing any issues? Or just seems like extra spam?

I've been debating declaring these targets 100% of the time, just like the _types target is now always exposed. That way the rule consistently outputs the same labels and the user invoking or depending on a target doesn't need to know if a transpiler is being used etc.

I think this is more likely to be extra spam – we only encountered the test failure when building on Windows or under QEMU.
On Windows there's a somewhat meaningful error message (ERROR(tools/test/windows/tw.cc:1302) ERROR: src/main/native/windows/process.cc(202): CreateProcessW("C:\tmp\hs7imhxj\execroot\workerd\bazel-out\x64_windows-opt\bin\src\workerd\api\http-test@ts_project_typecheck_test.sh.runfiles\workerd\src\workerd\api\http-test@ts_project_typecheck_test.sh"): %1 is not a valid Win32 application.) that suggests that the test is not being invoked correctly. That could be an issue with our build environment or an issue with how build_test works but not necessarily a problem with the test itself. I don't have access to a Windows environment so it's hard to tell for sure.

The approach in #721 looks most sensible to me (in terms of not having redundant targets being defined, we always use the default transpiler) but I also get that there's value in defining targets whether a custom transpiler is being used or not.

@jfirebaugh
Copy link
Contributor

I've been debating declaring these targets 100% of the time, just like the _types target is now always exposed.

FWIW, we (Figma) support this. We use a mix of swc and tsc, and the inconsistency of what targets ts_project generates has been a source of confusion.

@matthewjh
Copy link

I've been debating declaring these targets 100% of the time, just like the _types target is now always exposed.

FWIW, we (Figma) support this. We use a mix of swc and tsc, and the inconsistency of what targets ts_project generates has been a source of confusion.

We are in the same boat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants