Loading from multiple node_modules with version > 4.4.1

[duarten]

🐞 bug report

Affected Rule

nodejs_binary / nodejs_test

Is this a regression?

Yes, 4.4.1 works fine, but 4.4.2 and 4.4.3 are broken. This is likely related to the linker refactor.

Description

When a package is symlinked under node_modules due to, for example, the packge_name attribute of js_library, then node modules will be loaded from a different directory.

I noticed this with react, which complains when multiple instances are loaded.

We can see the problem from the following (edited) stack trace:

      <snip>
      at useMemo (node_modules/repro/node_modules/react/cjs/react.development.js:1531:20)
      at Component (../component/component.tsx:4:12)
      <snip>
      at act (node_modules/react-dom/cjs/react-dom-test-utils.development.js:1042:14)
      <snip>
      at runCLI (node_modules/@jest/core/build/cli/index.js:173:3)

Notice how we first load react from node_modules/react-dom, and later from node_modules/repro/node_modules.

The component package contains the following rule:

js_library(
    name = "component",
    package_name = "repro/component",
    visibility = ["//visibility:public"],
    deps = ["component-ts"],
)

Omitting the workspace name (repro) from the package_name seems to solve the problem (i.e., changing it to "component" or "blah/component").

🔬 Minimal Reproduction

I committed a reproduction here.

🔥 Exception or Error

In this case, react complains and the test fails.

🌍 Your Environment

Operating System:

  
  macOS
  

Output of bazel version:

  
  4.2.1
  

Rules_nodejs version:

  
  4.4.3
  

[duarten]

This is still happening with version 4.6.0.

@alexeagle, @gregmagolan any tips on how to fix this would be greatly appreciated :)

[alexeagle]

hi sorry, we've been 100% heads-down on the 5.x breaking changes and getting that release out. I doubt this is fixed there but we should at least verify it's still reproducible on 5.0.0-rc.0 which I'm going to cut todayish.

[duarten]

It still reproduces with 5.0.0-rc.1. I've updated the reproducer here.

[m0ar]

Could this possibly be related to my issue here? :thinking: https://github.com/bazelbuild/rules_nodejs/discussions/3340

[duarten]

It could be! If you are on 5.x.x it may also be related to https://github.com/bazelbuild/rules_nodejs/issues/3264, so you should try to set exports_directories_only to False in your yarn_install or npm_install rule.

[m0ar]

@duarten Thanks for the clue! :pray: Indeed solved the problem. Spend most of Friday trying to hunt this one out.

If you understand the mechanics behind the problem: will it be solved when #3264 is taken care of? I have a hard time figuring out if this is a bug or a side-effect of an optimisation. Anyway if it's the latter, it shouldn't be the default behaviour at least... :)

[duarten]

I think it's a bug and will be fixed in #3264, but I don't understand what exactly is causing the issue, I just had recently spent some time trying to figure a problem that was caused by it :)

[thesayyn]

Fixed by #3380

[duarten]

@thesayyn Can you reopen this issue? #3380 doesn't fix it (this is unrelated to exports_directories_only). You can run the reproducer here to check the test is still failing.

[thesayyn]

Ah sorry...

[gregmagolan]

I checked the repo rebased to stable HEAD. This looks like a side-effect of link_workspace_root = True, in the js_test.

In the call stack you can see it resolving into a deeply nested node_modules via the node_modules/repro symlink that the linker creates:

test.sh.runfiles/repro/node_modules/repro/node_modules/react/cjs/react.development.js

If I set that to false then the test passes,

        _jest_test(
            name = test_name,
            data = [jest_setup, jest_config, src] + deps + data,
            # link_workspace_root = True,
            args = args + ["--runTestsByPath", "$(rootpath %s)" % src],
            **kwargs
        )

Is the link_workspace_root intentional here?

[duarten]

It was intentional at some point, but it is indeed no longer necessary. Happy to close the issue unless there's something to attach to it like docs update or something.