Remove --bazel_patch_module_resolver

[dgp1130]

In the upgrade to rules_nodejs v3, we found that all nodejs_binary() and jasmine_node_test() rules would fail all import statements unless we added:

nodejs_binary(
    # ...
    templated_args = ["--bazel_patch_module_resolver"],
)

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 bazel-contrib/rules_nodejs#2388 (comment) to hopefully learn what we're doing wrong here to avoid needing this flag.

[dgp1130]

@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.

[dgp1130]

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 microsoft/TypeScript#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: bazel-contrib/rules_nodejs#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 bazel-contrib/rules_nodejs#2402 with my current understanding.

[dgp1130]

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.

1. 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.
2. Declarative shadow DOM becomes a problem again because we don't use a real NPM package with subpath exports like it is supposed to work. Normally you would just use a 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.
3. 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.
4. 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".

[dgp1130]

This issue has grown out of scope a bit so I filed #49 to migrate off link_workspace_root and #50 to migrate to ts_project(). This issue can just focus on --bazel_patch_module_resolver.

[dgp1130]

#50 has been resolved and the workspace is completely on ts_project(). However I think this is still blocked on removing link_workspace_root in #49.

[dgp1130]

--bazel_patch_module_resolver got dropped as part of moving to @aspect_rules_js in #48.