Closed dgp1130 closed 1 year ago
@alexeagle reached out to me about this. It seems that the solution is to switch from ts_library()
to ts_project()
, which should link things without requiring the patched module resolver. I did try that originally, but ran into some challenges and didn't push on it to hard. Revisiting now, I think I forgot to add rootDirs
options per ts_project()
docs (or didn't edit the path to be correct). With that change and enabling declaration = True
, ts_project()
rules are able to compose each other as I originally expected.
Switching the renderer
package to ts_project()
is able to build and pass all tests. However, switching the resource_injector
package successfully builds and tests itself, but breaks all example builds. Apparently, tests which invoke the nodejs_binary()
and bazel run
-ing the nodejs_binary()
both load as expected. However, running it as part of a build rule fails with "module not found" errors.
Alex mentioned that we need to use run_node()
/ npm_package_bin()
instead of ctx.actions.run()
/ genrule()
as it sets up node_modules/
in a usable fashion. With this, we can import named modules like yargs
.
link_workspace_root
is also necessary for absolute imports to be resolved, although the general recommendation from rules_nodejs
is to use relative imports because that is how Node actually works (rules_prerender/foo/bar
would not resolve the way we want if rules_nodejs
did nothing after all). I need to think about it a little more to decide if it is worth all the ../
paths, but regardless, link_workspace_root
still doesn't seem to fix imports, which still fail rules_prerender/...
paths. Relative imports are also a compiler error, though I'm not sure when that broke, as I'm pretty sure it worked previously.
Current work is in the ts-project
branch, I'll need to do more investigation here to understand what's going wrong.
Did a little more debugging with Alex and discovered that relative imports were broken at compile-time of TypeScript, except they would only compiled from a tool used by run_node()
or npm_package_bin()
. This is because tools invoked at build-time must be compiled in the host
or exec
configurations. I don't fully understand the difference between the two, however they put generated files in related directories. host
puts generated files in bazel-out/host/bin/...
, while exec
puts them in bazel-out/k8-${cfg}-exec-${id}/...
where ${cfg}
is the Bazel -c
flag (opt
, fastbuild
, dbg
, etc.), while ${id}
is a hashed ID value based on the compile-time flags.
TypeScript doesn't look in either of these directories based on the rootDirs
flag as copied from here:
Note: in order for TypeScript to resolve relative references to the bazel-out folder, we recommend that the base tsconfig contain a rootDirs section that includes all possible locations they may appear.
We hope this will not be needed in some future release of TypeScript. Follow https://github.com/microsoft/TypeScript/issues/37257 for more info.
For example, if the base tsconfig file relative to the workspace root is path/to/tsconfig.json then you should configure like:
"compilerOptions": { "rootDirs": [ ".", "../../bazel-out/darwin-fastbuild/bin/path/to", "../../bazel-out/k8-fastbuild/bin/path/to", "../../bazel-out/x64_windows-fastbuild/bin/path/to", "../../bazel-out/darwin-dbg/bin/path/to", "../../bazel-out/k8-dbg/bin/path/to", "../../bazel-out/x64_windows-dbg/bin/path/to", ] }
See some related discussion including both “rootDirs” and “paths” for a monorepo setup using custom import paths: https://github.com/bazelbuild/rules_nodejs/issues/2298
The solution for host
, is to simply add a new path here, which fixes TypeScript compilation of relative imports in the host
configuration. Also made a PR to update rules_nodejs
docs about this.
exec
is a bit trickier, since the ID value is not stable, so it can't be easily hard-coded into the tsconfig
. Fortunately the easy solution is to just change our build tools to use the host
configuration rather than the exec
configuration.
With that, relative imports compile and run as a build tool. However, absolute imports still fail. They will compile successfully in TypeScript, but fail when executed in nodejs_binary()
, even with link_workspace_root = True
. I suspect it is a similar problem, that all the files are under bazel-out/host/bin
and the rules_prerender
workspace name needs to be linked to that directory. Filed https://github.com/bazelbuild/rules_nodejs/issues/2402 with my current understanding.
Quick update here. I've spent some more time digging into migrating to ts_project()
and made some progress and also better understand the issues that come with it.
import 'webdriverio';
doesn't work with ts_project()
because that is not actually a module (no import
/ export
). Even if it compiled successfully, it would fail at runtime since NodeJS wouldn't have anything to import. I managed to make it work with a project reference, which feels like a hack but I don't have a better solution here.js_library(package_name = "rules_prerender")
and include a package.json
with some exports
field. But that doesn't work here because declarative shadow DOM is a prerender_component()
with more than just JS files for a NodeJS runtime. That component needs to be depended upon via deps
, while the typical @npm//rules_prerender
package goes into lib_deps
. This necessitates two different Bazel targets using two different rules despite being part of the same rules_prerender
NPM package. I was able to make this work by defining package_path = "rules_prerender"
in both the declarative shadow DOM prerender_component()
and //packages/rules_prerender
. rules_nodejs
is ok with this and will merge the definitions together as long as they map to the same Bazel package, which is an awkward constraint.ts_library()
generated two different JS versions, one using CommonJS and the other with ESM. The former was used for prerender code, while the latter was using for client-side scripts. Migrating everything to ts_project()
means that everything just uses the module specified in the given tsconfig.json
(CommonJS in my case by default). As a result, client side scripts need their own tsconfig.json
just to switch the module to ESM. I think this somewhat desirable anyways, since JS targeting the browser and JS targeting Node have different environments and often different configs as a result.link_workspace_root = True
breaks ts_project()
targets which depend on //packages/rules_prerender
. This is because //packages/rules_prerender
defines the module name rules_prerender
mapping to the Bazel package packages/rules_prerender
. However link_workspace_root = True
defines a module with the same name as the workspace (which also happens to be rules_prerender
mapped to the root Bazel package ""
. These two values conflict and break. This means we either need to move off link_workspace_root
and switch to relative imports or rename the workspace to not match the NPM package. I kinda want to rename the NPM package from rules_prerender
to @rules_prerender/build
so there's space for other official packages, so maybe that's the right solution?I feel like a lot of the declarative shadow DOM issue would go away if I could depend on rules_prerender
like the NPM package is supposed to work. That is, if I could use @npm//rules_prerender
and @npm//rules_prerender:declarative_shadow_dom
, then a lot of the problem would go away (I think?). Unfortunately I can't really do in a workspace-local context. I was thinking it would be really cool if I could use npm_install(links = { "rules_prerender": "//:pkg" })
and then actually use @npm//rules_prerender
in the examples directory. This doesn't really work because DeclarationInfo
isn't re-exported since that gets dropped in the pkg_npm()
rule. It would be awesome if some linking to a pkg_npm()
rule worked identically to actually installing that package. I think in practice that's not quite possible since it would need to happen after Starlark loading time so load("@npm//rules_prerender:index.bzl", "prerender_component")
would be impossible with this scheme since it needs to build //:pkg
before it can load :index.bzl
. I think most of the work there is also done in workspace rules, so it's probably not totally feasible to do all the same work in a typical analysis rule. Even if it only worked an analysis time though would still be a huge win. I might follow up with rules_nodejs
and see if there's anything possible there. I'm not totally convinced this solves the declarative shadow DOM problem, I feel like I still need two js_library()
targets with package_name = "rules_prerender"
.
This issue has grown out of scope a bit so I filed https://github.com/dgp1130/rules_prerender/issues/49 to migrate off link_workspace_root
and https://github.com/dgp1130/rules_prerender/issues/50 to migrate to ts_project()
. This issue can just focus on --bazel_patch_module_resolver
.
ts_project()
. However I think this is still blocked on removing link_workspace_root
in #49.--bazel_patch_module_resolver
got dropped as part of moving to @aspect_rules_js
in #48.
In the upgrade to
rules_nodejs
v3, we found that allnodejs_binary()
andjasmine_node_test()
rules would fail allimport
statements unless we added:I'm still unclear on why this is necessary and what the linker is not doing that requires this in the first place. Posted a question to https://github.com/bazelbuild/rules_nodejs/issues/2388#issuecomment-757420705 to hopefully learn what we're doing wrong here to avoid needing this flag.