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

--nobazel_patch_module_resolver should be the default #2125

Closed
alexeagle opened this issue Aug 17, 2020 · 2 comments
Closed

--nobazel_patch_module_resolver should be the default #2125

alexeagle opened this issue Aug 17, 2020 · 2 comments
Assignees
@alexeagle
Labels
cleanup Tech debt, resolving it improves our own velocity
Milestone
3.0

Comments

@alexeagle
Copy link
Collaborator

In my last change 7d070ff#diff-ccd16b070808615570daf5b61d659258 I wrote

As a later step we'll change default for patch_module_resolver to false (could be for 2.0 or 3.0 depending on how breaking it looks)

we should do this for 3.0
@alexeagle alexeagle added this to the 3.0 milestone Aug 17, 2020
@alexeagle alexeagle added cleanup Tech debt, resolving it improves our own velocity package:builtin labels Aug 17, 2020
@thesayyn
Copy link
Collaborator

Finally, it is happening. 🎉 Module resolver patching has been a troublemaker.

This was referenced Dec 1, 2020
Closed
Closed
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 2, 2020
refactor: default to nobazel_patch_module_resolver
269718a 
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:
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, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]

Fixes bazel-contrib#2125
@alexeagle alexeagle mentioned this issue Dec 2, 2020
Closed
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 2, 2020
refactor: default to nobazel_patch_module_resolver
01c0665 
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:
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, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]

Fixes bazel-contrib#2125
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 2, 2020
refactor: default to nobazel_patch_module_resolver
d1e485d 
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:
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, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]

Fixes bazel-contrib#2125
alexeagle pushed a commit to alexeagle/rules_sass that referenced this issue Dec 2, 2020
prepare for breaking change in rules_nodejs
a8871d8 
bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.
alexeagle pushed a commit to alexeagle/rules_sass that referenced this issue Dec 2, 2020
prepare for breaking change in rules_nodejs
4f503ee 
bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.
@alexeagle alexeagle mentioned this issue Dec 2, 2020
Merged
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 2, 2020
refactor: default to nobazel_patch_module_resolver
3945066 
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:
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, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]

Fixes bazel-contrib#2125
alexeagle pushed a commit to alexeagle/rules_sass that referenced this issue Dec 4, 2020
prepare for breaking change in rules_nodejs
856d1c5 
bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.
nex3 pushed a commit to bazelbuild/rules_sass that referenced this issue Dec 4, 2020
@alexeagle
prepare for breaking change in rules_nodejs (#126)
931b54a 
bazel-contrib/rules_nodejs#2125 is going to turn off the patching of the require() function by default. The reason is that it breaks compatibility with many node programs. However rules_sass depends on this, and also has at least conceptual sharing with google3 which uses the patched require() as well.

This change is a no-op for rules_nodejs 2.x where the default is true, but is required with rules_nodejs 3.0 where it will be false.
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 9, 2020
refactor: default to nobazel_patch_module_resolver
fa88f38 
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:
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, try opting-in to the require patches with templated_args = ["--bazel_patch_module_resolver"]

Fixes bazel-contrib#2125
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 11, 2020
refactor: make dependencies on --bazel_patch_module_resolver explicit
8ff9f66 
We are about to flip the default for this flag in bazel-contrib#2125 so we need to explicitly opt-in in places where we need the behavior.
Ideally most of these should be fixed, this would be necessary if we ever want to deprecate the module resolver patches
@alexeagle alexeagle mentioned this issue Dec 11, 2020
Merged
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Dec 11, 2020
refactor: make dependencies on --bazel_patch_module_resolver explicit
f013f04 
We are about to flip the default for this flag in bazel-contrib#2125 so we need to explicitly opt-in in places where we need the behavior.
Ideally most of these should be fixed, this would be necessary if we ever want to deprecate the module resolver patches
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue 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
gregmagolan pushed a commit that referenced this issue Dec 11, 2020
@gregmagolan
refactor: make dependencies on --bazel_patch_module_resolver explicit
0203c06 
We are about to flip the default for this flag in #2125 so we need to explicitly opt-in in places where we need the behavior.
Ideally most of these should be fixed, this would be necessary if we ever want to deprecate the module resolver patches
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue 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 issue 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
@alexeagle
Copy link
Collaborator Author

Fixed by #2347

@gonzojive gonzojive mentioned this issue Jul 27, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@alexeagle alexeagle

Labels
cleanup Tech debt, resolving it improves our own velocity
Projects
None yet
Milestone
3.0
Development

No branches or pull requests
2 participants
@alexeagle @thesayyn