node_modules attribute in jasmine_node_test does not take effect

[mprobst]

A long time back, tsickle was ported to bazel. Part of its test suite are (somewhat odd) targets that run its tests against TypeScript 2.4-2.8, for example:
https://github.com/angular/tsickle/tree/master/test_ts24

These basically create a local node_modules directory and then run the test suite against that, like so:
jasmine_node_test(
name = "golden_test",
srcs = [
"check_ts_version.js",
"//test:golden_test_lib",
],
node_modules = ":node_modules",
data = ["//:test_files"],
)

This is all nice and fine, except for the bit where it totally doesn't work and doesn't actually run the tests against the right TypeScript version (argh!).

What happens is that bazel + Jasmine build rules create a layout like so:
bazel-bin/test_ts24/golden_test.runfiles/tsickle/test_ts24
bazel-bin/test_ts24/golden_test.runfiles/tsickle/test/golden_tsickle_test.js
...
bazel-bin/test_ts24/golden_test.runfiles/tsickle/test_ts24/node_modules/typescript/...

The nested typescript package does have the right version:
$ jq .version bazel-bin/test_ts24/golden_test.runfiles/tsickle/test_ts24/node_modules/typescript/package.json
"2.4.2"

But as you can see, golden_tsickle_test.js does not have it as a parent, and so when that loads 'typescript', it just gets the latest version that's in the top level node_modules folder.

So, in a nutshell, ever since we moved tsickle to bazel, none of these tests tested what they claimed they tested :-(

I don't quite know how to fix this though. I've created a partial PR for rules_nodejs in #236. That does create a symlink tree "node_modules" in the right location. However, nodejs helpfully still de-references all symlinks when loading files, so it still bypasses that, and I still end up with TS2.9 being used for all my tests.

[mprobst]

See angular/tsickle#837 for a repro of the breakage.

[gregmagolan]

@mprobst I've run into a similar node_modules resolution problem when working to get angular building from bazel managed node_modules and built from source in angular-bazel-example. I spent a few days in the weeds with node_module resolution in nodejs_binary and nodejs_test and reworked the resolution in the two so that it works correctly. Still have to discuss this further with @alexeagle but here is how I think it should work: #240. This may help in this instance as well as it allows you two pass multiple node_modules with a node_modules_list attribute to nodejs_binary and nodejs_test.

For reference, angular build with bazel managed deps (@angular_deps//:node_modules) is here: angular/angular#24663. And angular-bazel-example with angular built from source (instead of using npm bundles) is here: angular/angular-bazel-example#160

I'll take a closer look at your issue today and try and understand what is going on.

[mprobst]

Thanks for looking into it @gregmagolan.

[gregmagolan]

@mprobst My changes in #240 seemed to have resolved the issue of the wrong type_script version being resolved in the jasmine_node_test. I pushed a commit on top of your repro branch to my tsickle fork: https://github.com/gregmagolan/tsickle/tree/nodejs-ver-fix. The TSICKLE_EXPECTED_TS_VERSION check you added to golden_tsickle_test.ts now passes. The tests themselves fail but that may be because the golden files need updating now that the right typescript is being resolved??

Because the :node_modules now takes effect in the jasmine_node_test, the test fails to import required packages (such as minimist) from the root //:node_modules unless you use the node_modules_list attribute which got added in #240 as well: node_modules_list = [":node_modules", "//:node_modules"], for those jasmine_node_tests. These are ordered at resolve time so typescript will still resolved to :node_modules.

Interesting that you came across this around the same time I ran into a similar issue building angular with bazel managed deps (in an external @angular_deps//:node_modules that is created with the yarn_install repository rule). In my case, I was getting resolves to the root workspace angular/node_modules when I was expecting to resolve to @angular_deps//:node_modules.

When @alexeagle is back next week I'll go over the changes in #240 with him.

[alexeagle]

We discussed the changes in #240 and believe that having multiple node_modules combined together should not be necessary. Using npm's dependency mechanism, we should always be able to install a node_modules tree that has the right versions of everything. However we don't currently look up the correct parent when resolving in node_modules. Will work on it more when Greg is back from vacation next week.

[alexeagle]

This should be fixed by #253

We now correctly use the parent (the file with the require statement) path to first look up child node_module` directories underneath it.