Add option to include transitive typings

[strmer15]

🚀 feature request

Relevant Rules

tsc_test

Description

In 4.6.0, commit https://github.com/bazelbuild/rules_nodejs/commit/6132f152e0877133e80cba991855b449b8770e4e removed the typings from transitive dependencies. All of our tests that depend on other libraries within our monorepo are now failing. To fix it, we'd need to add in every BUILD file a specific filegroup for types, which is somewhat cumbersome and verbose.

I understand why the change was made, but it's not relevant for our setup at the moment - is there some way that an option could be provided so we can turn the default behavior back on?

Describe the solution you'd like

An option somewhere to either automatically find the typings for all transitive dependencies so they could be easily included in the data for the tests, or a flag to turn on the old behavior for those who aren't using a separate transpiler.

Describe alternatives you've considered

Writing a number of filegroups that would get the typings and add them to the data for the tests, which would be cumbersome and verbose for a large monorepo.

[alexeagle]

Hmm, sorry about that. Obviously we should have reverted the mistake within the same release it was made. Options at this point:

1. re-revert the change until 5.0 and then break you in the same way. Not really a solution, and also the new transpile-only mode of ts_project depends on it, so we'd have to back that feature out as well.
2. you can just patch this in your repo. Make a file like /js_library.runfiles_typings.patch containing

diff internal/js_library/js_library.bzl internal/js_library/js_library.bzl
index dfe4edaa..1e46eba0 100644
--- internal/js_library/js_library.bzl
+++ internal/js_library/js_library.bzl
@@ -228,7 +228,7 @@ def _impl(ctx):
         # We do not include typings_depsets in the runfiles because that would cause type-check actions to occur
         # in every development workflow.
         transitive_files = depset(
-            transitive = files_depsets,
+            transitive = files_depsets + typings_depsets,
         ),
     )
     deps_runfiles = [d[DefaultInfo].default_runfiles for d in ctx.attr.deps]

and add patches = ["//js_library.runfiles_typings.patch"] in your http_archive(name = "build_bazel_rules_nodejs") call

3. change all your loads of tsc_test to come from a macro within your repo (one-time bulk change), then make the filegroup fix within that macro. I think it's a good practice to have a large monorepo load things through a local point of indirection, though I understand it's annoying to be forced to do that
4. We introduce something in js_library to make this mode available. You wouldn't want to change all your callsites so I think we'd have to introduce a custom configuration flag. This is sad extra complexity given that you can now use ts_project to do a transpile-only use case, and the mode wouldn't be very discoverable for new users. But given that you were broken by this revert I think it's a reasonable request that we do it anyway.

My preference here is 2 but if you feel strongly about it we could do 4.

[strmer15]

@alexeagle Thanks for all the information!

I agree, option 2 was one of the options I was thinking about but didn't understand how to patch these internal files before. We actually have a macro already so I stumbled on option 3 while trying some things out. I think it will probably be the best long term way to fix this for us.

I'll close this since it seems like there's a couple of simple workarounds and option 4 seems like a lot of extra work without much point. Thanks again!

[duarten]

I think this should be reopened. It's a regression to have something depend on js_library but not have access to the typings produced by js_library dependencies. Would it be possible to set a flag somewhere indicating whether a js_library is depending on a vanilla ts_project, or one with a custom transpiler specified?

[duarten]

Maybe the fix is simpler? If a js_library depends on a ts_project rule without the transpiler, then tsc has already ran and we don't need to optimize out the decl files from the runfiles. If js_library depends on just the target from ts_project rule with a transpiler, then it won't have a DeclarationInfo. on the other hand, if I want to explicitly include decls in the js_library, then it needs to depend on the ts_project target as well as it's _typecheck outputs. In that case, because of that dependency, tsc will also run (I believe). So does this mean that when js_library sees a DeclarationInfo, tsc has already run and it's okay to include the decls in the runfiles?

[alexeagle]

What change would you propose making then? I can't think of anything aside from a global flag, or an added attribute on js_library that you'd have to add on many/all of them.

[duarten]

Well, it was more a question than a statement, but if what I wrote is correct (if js_library see a DeclarationInfo, a tsc execution has already taken place), then the declarations should always exist in the runfiles.

[alexeagle]

That's not how it works though. It depends what outputs you request from the js_library. If it always propagates declarationinfo into runfiles then it causes the eager type checking which was why that change is reverted. That's why one workaround is to request the types explicitly with an output_group.

[duarten]

That's surprising though. If I have a ts_project rule named "foo", using a transpiler, and a js_library that depends both on foo and on foo_typings, won't Bazel build the js_library dependencies whenever we run another target that depends on the js_library?

[alexeagle]

Not necessarily. It depends which providers that dependency on the js_library asks for. You have to consider how the action graph is finer-grained and different shape, and is "lowered" into the dependency graph. Execution depends on the action graph, not the dependency graph.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had any activity for 6 months. It will be closed if no further activity occurs in 30 days. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

[github-actions[bot]]

This issue was automatically closed because it went 30 days without any activity since it was labeled "Can Close?"