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.

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

Sourcemaps is broken after v3 upgrade #2520

Closed
mitul45 opened this issue Mar 9, 2021 · 3 comments · Fixed by #2538
Closed

Sourcemaps is broken after v3 upgrade #2520

mitul45 opened this issue Mar 9, 2021 · 3 comments · Fixed by #2538
Labels

Comments

@mitul45
Copy link

mitul45 commented Mar 9, 2021

🐞 bug report

Affected Rule

The issue is caused by the rule: nodejs_binary

Is this a regression?

Yes, the previous version in which this bug was not present was: 2.3.1

Description

After upgrading the rules_nodejs from 2.3.1 to 3.2.2 the sourcemaps have stopped working. In case the program is throwing an error,

Before:

Error: here
    at Object.<anonymous> (index.ts:1:7)

After:

throw new Error("here");
^

Error: here
    at Object.<anonymous> (/private/var/tmp/_bazel_mshah/fa060aa929cca939406f1ed94045b096/execroot/example/bazel-out/darwin-fastbuild/bin/bin.sh.runfiles/example/index.js:2:7)

As you can see, before this upgrade, the file name displayed in logs is a .ts file and it points to original source, but after the upgrade, the logs contain reference to the compiled .js file.

🔬 Minimal Reproduction

Link to repo: https://github.com/mitul45/bazel-example

To see actual stack-trace, uncomment lines to use rules_nodejs v2.3.1 and downgrade @bazel/typescript to 2.3.1 (yarn upgrade @bazel/[email protected])

🌍 Your Environment

Operating System:

  
macOS Catalina
10.15.7
  

Output of bazel version:

  
/git/bazel-example (main)  >> yarn bazel version
yarn run v1.22.5
$ /Users/mshah/git/bazel-example/node_modules/.bin/bazel version
Bazelisk version: v1.7.5
Build label: 4.0.0
Build target: bazel-out/darwin-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Thu Jan 21 07:38:50 2021 (1611214730)
Build timestamp: 1611214730
Build timestamp as int: 1611214730
✨  Done in 1.39s.
  

Rules_nodejs version:

(Please check that you have matching versions between WORKSPACE file and @bazel/* npm packages.)

  
3.2.2
  

Anything else relevant?
Nope

@joeljeske
Copy link
Contributor

Looks like it could be related to this comment?

#2448 (comment)

Can you try passing that flag to see if it fixes it? Then the real fix is to do as @alexeagle suggested; fix the source maps even without that flag.

@mitul45
Copy link
Author

mitul45 commented Mar 15, 2021

Thanks @joeljeske, adding

        templated_args = [
            "--bazel_patch_module_resolver",
        ],

to bin rule fixed the sourcemaps for me! Do you know how can I use --require source-map-support instead?

@alexeagle
Copy link
Collaborator

adding this made the stack trace show .ts files

templated_args = ["--node_options=--require=source-map-support/register"]

diff --git a/BUILD.bazel b/BUILD.bazel
index e922e53..d5bc55c 100644
--- a/BUILD.bazel
+++ b/BUILD.bazel
@@ -11,5 +11,6 @@ ts_library(
 nodejs_binary(
     name = "bin",
     data = [":sources"],
-    entry_point = ":index.ts"
+    entry_point = ":index.ts",
+    templated_args = ["--node_options=--require=source-map-support/register"],
 )

Note that we vendored a copy of the source-map-support into the rules_nodejs "built-in" distribution, so this is always available to add even if your app doesn't depend on source-map-support itself.

Perhaps we can just always turn this on, like we did in the case of the patched module resolver, I'll TAL

alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Mar 17, 2021
We previously did this when patch_module_resolver was the default, because it had some extra (unrelated) logic at the end to install it.
When we flipped the default in 3.0, we accidentally dropped the feature.

This just reinstates it using the source-map-support/register feature.
Similarly to our prior implementation, we just call our own vendored source-map-support so users don't have to think about it.

Fixes bazel-contrib#2520
alexeagle pushed a commit to alexeagle/rules_nodejs that referenced this issue Mar 17, 2021
We previously did this when patch_module_resolver was the default, because it had some extra (unrelated) logic at the end to install it.
When we flipped the default in 3.0, we accidentally dropped the feature.

This just reinstates it using the source-map-support/register feature.
Similarly to our prior implementation, we just call our own vendored source-map-support so users don't have to think about it.

Also fix missing docs for npm_package_bin attrs stdout,stderr,exit_code_out

Fixes bazel-contrib#2520
@alexeagle alexeagle added the bug label Mar 17, 2021
alexeagle added a commit that referenced this issue Mar 17, 2021
We previously did this when patch_module_resolver was the default, because it had some extra (unrelated) logic at the end to install it.
When we flipped the default in 3.0, we accidentally dropped the feature.

This just reinstates it using the source-map-support/register feature.
Similarly to our prior implementation, we just call our own vendored source-map-support so users don't have to think about it.

Also fix missing docs for npm_package_bin attrs stdout,stderr,exit_code_out

Fixes #2520
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants