Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: explicitly use runfiles helper #2341

Merged
merged 1 commit into from
Dec 11, 2020
Merged

refactor: explicitly use runfiles helper #2341

merged 1 commit into from
Dec 11, 2020

Conversation

alexeagle
Copy link
Collaborator

@google-cla google-cla bot added the cla: yes label Dec 11, 2020
refactor: explicitly use runfiles helper
36fdb9a 
This reduces our dependence on the monkey-patched require() that makes NodeJS look at the runfiles manifest when resolving modules.
Pre-factoring for changing the default of --bazel_patch_module_resolver to false
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this pull request Dec 11, 2020
refactor: bazel_patch_module_resolver default to false
92ad73c 
This turns off our monkey-patches for require() for nodejs_binary, nodejs_test, and macros like jasmine_node_test.
Note it was previously disabled for npm_package_bin and generated index.bzl binaries in bazel-contrib#1440

BREAKING CHANGE:
By default, we no longer patch the require() function, instead you should rely on the linker to make node modules resolvable at the standard location
if this breaks you, the quickest fix is to flip the flag back on a nodejs_binary/nodejs_test/npm_package_bin with `templated_args = ["--bazel_patch_module_resolver"]`, see bazel-contrib#2344 as an example.
Another fix is to explicitly use our runfiles helper library, see bazel-contrib#2341 as an example.

Fixes bazel-contrib#2125
@alexeagle alexeagle merged commit f1f700f into bazel-contrib:3.x Dec 11, 2020
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this pull request Dec 11, 2020
refactor: bazel_patch_module_resolver default to false
4feaa88 
This turns off our monkey-patches for require() for nodejs_binary, nodejs_test, and macros like jasmine_node_test.
Note it was previously disabled for npm_package_bin and generated index.bzl binaries in bazel-contrib#1440

BREAKING CHANGE:
By default, we no longer patch the require() function, instead you should rely on the linker to make node modules resolvable at the standard location
if this breaks you, the quickest fix is to flip the flag back on a nodejs_binary/nodejs_test/npm_package_bin with `templated_args = ["--bazel_patch_module_resolver"]`, see bazel-contrib#2344 as an example.
Another fix is to explicitly use our runfiles helper library, see bazel-contrib#2341 as an example.

Fixes bazel-contrib#2125
alexeagle pushed a commit that referenced this pull request Dec 11, 2020
@alexeagle
refactor: bazel_patch_module_resolver default to false
fdde32f 
This turns off our monkey-patches for require() for nodejs_binary, nodejs_test, and macros like jasmine_node_test.
Note it was previously disabled for npm_package_bin and generated index.bzl binaries in #1440

BREAKING CHANGE:
By default, we no longer patch the require() function, instead you should rely on the linker to make node modules resolvable at the standard location
if this breaks you, the quickest fix is to flip the flag back on a nodejs_binary/nodejs_test/npm_package_bin with `templated_args = ["--bazel_patch_module_resolver"]`, see #2344 as an example.
Another fix is to explicitly use our runfiles helper library, see #2341 as an example.

Fixes #2125
@WesleyYue
Copy link

WesleyYue commented May 20, 2021 •
edited
Loading

Is there an example of how we can apply this fix without resorting to bazel_patch_module_resolver with ts_project compiled code? I have a minimal example here reproducing the breakage (note it's the js_module_not_found branch). Is there something we can put in tsconfig.json to help tsc resolve the paths?

Also fwiw, it seems like this could be a really nasty bug for someone that tired using >3.0.0 first without needing to migrate. I would expect the code in the repro to work out of the box, and the error itself is not very searchable. Not sure how I would've found the fix if I didn't know that migrating versions was the cause. The examples with ts_project also doesn't have any that involve nodejs_binary afaik.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbedard jbedard jbedard approved these changes

@gregmagolan gregmagolan Awaiting requested review from gregmagolan gregmagolan is a code owner

@mattem mattem Awaiting requested review from mattem mattem is a code owner

@mrmeku mrmeku Awaiting requested review from mrmeku mrmeku is a code owner

@soldair soldair Awaiting requested review from soldair soldair is a code owner

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alexeagle @WesleyYue @jbedard