Initial sourcemaps: upload from target dir #1

waltjones · 2020-08-10T16:28:38Z

Description of the change

Initial implementation to:

scan a target dir for minified js and correctly associated map files
find and load original source files
upload via Rollbar API
Report status, progress, success and errors

Reviewers: test/fixtures/ contains a lot of files that dominate the diff. However, changes are cleanly separated by commit. If it's hard to navigate the overall PR, go to the specific commit.

Styleguide:

Written in plain Node 8 syntax. No transpile (e.g. es6 or TypeScript)
With semicolons, in keeping with other Rollbar Node.js projects

Test/Evaluation:
When published to npm, install globally using npm install -g rollbar-cli

For now, clone repo and run npm link. The rollbar command will be globally available.

Known Issues:
ch72220: Upload fails due to filename parsing bug. Including original sources can cause the entire upload request to fail due to an API bug.
ch72221: Original sources option disabled. The spec doesn't provide an option to send original sources, and doing so without a way to turn it off causes several issues that would render the CLI unusable by many users.

Command line spec has been updated to match Mrunal spec. https://rollbar.quip.com/amFQAoKENQ81/Design-of-rollbar-cliWIP

rollbar-cli upload-sourcemaps --help (source map options)

rollbar-cli upload-sourcemaps <path> [options]

upload sourcemaps

Options:
  --version       Show version number                                  [boolean]
  -v, --verbose   Verbose status output                                [boolean]
  -q, --quiet     Silent status output                                 [boolean]
  --help          Show help                                            [boolean]
  --access-token  Access token for the Rollbar API           [string] [required]
  --url-prefix    Base part of the stack trace URLs          [string] [required]
  --code-version  Code version string must match value in the Rollbar item
                                                             [string] [required]

Tasks:

Change command line interface
Test automation
CI integration
Update readme

Coverage report

-----------------|---------|----------|---------|---------|-------------------
File             | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s 
-----------------|---------|----------|---------|---------|-------------------
All files        |   95.51 |    83.93 |   94.29 |   95.51 |                   
 src             |     100 |      100 |     100 |     100 |                   
  index.js       |     100 |      100 |     100 |     100 |                   
 src/common      |   95.45 |    82.35 |   85.71 |   95.45 |                   
  output.js      |   95.24 |    88.89 |   88.89 |   95.24 | 32                
  rollbar-api.js |   95.65 |       75 |      80 |   95.65 | 15                
 src/sourcemaps  |   95.38 |    84.62 |     100 |   95.38 |                   
  command.js     |     100 |      100 |     100 |     100 |                   
  scanner.js     |   94.12 |    76.92 |     100 |   94.12 | 64,70,100-101,122 
  uploader.js    |   96.77 |      100 |     100 |   96.77 | 45                
-----------------|---------|----------|---------|---------|-------------------

Fixtures

The tests use the build output from typical React and Angular projects.

Examples

React
rollbar-cli upload-sourcemaps dist -access-token 638d... --url-prefix 'http://localhost:3000/' --code-version react16

[Found ] dist/f4fa9cd60b1b9860d730.worker.js
         f4fa9cd60b1b9860d730.worker.js.map
         1 original sources
         valid
[Found ] dist/main.js
         main.js.map
         48 original sources
         valid
[Upload] f4fa9cd60b1b9860d730.worker.js.map
         Upload successful
[Upload] main.js.map
         Upload successful

Angular 8
rollbar-cli upload-sourcemaps dist/my-app --access-token 638d... --url-prefix 'http://localhost:3000/' --code-version angular8v1 -s

[Found ] dist/my-app/es2015-polyfills.js
         es2015-polyfills.js.map
         251 original sources
         valid
[Found ] dist/my-app/main.js
         main.js.map
         9 original sources
         valid
[Found ] dist/my-app/polyfills.js
         polyfills.js.map
         86 original sources
         valid
[Found ] dist/my-app/runtime.js
         runtime.js.map
         1 original sources
         valid
[Found ] dist/my-app/styles.js
         styles.js.map
         4 original sources
         valid
[Found ] dist/my-app/vendor.js
         vendor.js.map
         207 original sources
         valid
[Upload] es2015-polyfills.js.map
         Upload successful
[Upload] main.js.map
         Upload successful
[Upload] polyfills.js.map
         Upload successful
[Upload] runtime.js.map
         Upload successful
[Upload] styles.js.map
         Upload successful
[Upload] vendor.js.map
         Upload successful

Angular 9
rollbar-cli upload-sourcemaps dist/angular9 --access-token 638d... --url-prefix 'http://localhost:3000/' --code-version angular9v1 -s

[Found ] dist/angular9/main-es2015.js
         map not found
[Found ] dist/angular9/main-es5.js
         main-es5.js.map
         6 original sources
         valid
[Found ] dist/angular9/polyfills-es2015.js
         map not found
[Found ] dist/angular9/polyfills-es5.js
         polyfills-es5.js.map
         275 original sources
         valid
[Found ] dist/angular9/runtime-es2015.js
         map not found
[Found ] dist/angular9/runtime-es5.js
         runtime-es5.js.map
         1 original sources
         valid
[Found ] dist/angular9/styles-es2015.js
         map not found
[Found ] dist/angular9/styles-es5.js
         styles-es5.js.map
         3 original sources
         valid
[Found ] dist/angular9/vendor-es2015.js
         index.js.map
[Error ] Error parsing map file: ENOENT: no such file or directory, open '/home/walt/projects/rollbar/angular/angular9/dist/angular9/index.js.map'
[Found ] dist/angular9/vendor-es5.js
         vendor-es5.js.map
         202 original sources
         valid
[Upload] null
         skip: dist/angular9/main-es2015.js
[Upload] main-es5.js.map
[Error ] Error: Source file(s) "src/app/app" do not exist in the given source map!
[Upload] null
         skip: dist/angular9/polyfills-es2015.js
[Upload] polyfills-es5.js.map
         Upload successful
[Upload] null
         skip: dist/angular9/runtime-es2015.js
[Upload] runtime-es5.js.map
         Upload successful
[Upload] null
         skip: dist/angular9/styles-es2015.js
[Upload] styles-es5.js.map
         Upload successful
[Upload] index.js.map
         skip: dist/angular9/vendor-es2015.js
[Upload] vendor-es5.js.map
         Upload successful

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Development

Lint rules pass locally
The code changed/added as part of this pull request has been covered with tests
All tests related to the changed code pass in development

Code review

This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
"Ready for review" label attached to the PR and reviewers mentioned in a comment
Changes have been reviewed by at least one other engineer
Issue from task tracker has a link to this pull request

vselvarajijay · 2020-08-10T16:50:16Z

LGTM - I think this is a good start for source maps..

mrunalk · 2020-08-11T03:52:34Z

Nice work @waltjones on getting initial patch out the door. Since it's WIP I won't add myself to the review yet. But initial questions looking at the patch,

You mentioned that it's written in plain Node 8 syntax(even though it supports 8, 10 and 12) but Node 8 is EOLed. Will that be a concern in near future?
How about using Github CI instead of travis CI? I see their CI getting better over time.

waltjones · 2020-08-12T19:16:32Z

@mrunalk Node 8 just means that it uses no Node features added in Node 9+. It does not mean that it isn't compatible with those versions. (It is compatible with later versions.) Since this runs on the user's installed Node, it makes sense to be inclusive as reasonably possible.

The travis YML was included, but there is nothing in the code or any other files tying this to travis. Once a CI is decided, we can just update to use that CI. (BTW, I have no concerns with Github Actions CI.)

vselvarajijay

LGTM - but did you intend on checking in the build folder?

waltjones · 2020-08-25T16:40:38Z

@vselvarajijay Yes, those are actually test fixtures. Much easier to see that in tree view. Much harder in PR view.

=> https://github.com/rollbar/rollbar-cli/tree/wj-initial-sourcemaps/test/fixtures/builds

mrunalk · 2020-08-25T21:51:29Z

src/sourcemaps/command.js

+  })
+  .option('D', {
+    alias: 'dry-run',
+    describe: 'Scan and validate source maps without uploading',


I like the option of dry run.

mrunalk · 2020-08-25T21:54:12Z

src/sourcemaps/scanner.js

+const path = require('path');
+const BasicSourceMapConsumer = require('source-map/lib/source-map-consumer').BasicSourceMapConsumer;
+
+class Scanner {


Will this scanner work if sourcemap and sources are in a totally different directory? I see only one targetPath.

It uses sourceMappingURL to determine the source map location.

Can there be a case when sourceMappingURL is not there?

mrunalk · 2020-08-25T22:08:12Z

src/sourcemaps/scanner.js

+      mappings[m.generatedLine] = true;
+    });
+
+    return true;


It seems like we are always returning true suggesting maps are always valid. I am guessing there will also be a case when map is invalid.

For now, the only non-true result of this method is if BasicSourceMapConsumer throws. At some point, it would make sense to use the iteration of consumer to do more.

I've refactored the validate method to make it more apparent that we're catching and reporting errors from BasicSourceMapConsumer. (The errors no longer bubble up to the outer catch.) Also, validate() now returns an errors array that is empty on success.

src/sourcemaps/scanner.js

mrunalk · 2020-08-25T22:52:15Z

src/sourcemaps/scanner.js

+      file.mapPathName = mapPath;
+      file.sourceMappingURL = true;
+    } else {
+      output.warn('', 'map not found');


I am assuming control will reach here when map path is not in the minified file. In that case we would have to manually find the sourcemap in the directory right?

What I meant here is that if we don't find map path do we want to prompt the user to enter the path manually but probably that won't be intuitive.

mrunalk

@waltjones, did you test if this works for TypeScript and Babel sourcemaps? If not it will be good to add tests for those as well.

mrunalk · 2020-08-26T22:31:57Z

src/sourcemaps/scanner.js

+const path = require('path');
+const BasicSourceMapConsumer = require('source-map/lib/source-map-consumer').BasicSourceMapConsumer;
+
+class Scanner {


Can there be a case when sourceMappingURL 23D3 is not there?

mrunalk · 2020-08-26T22:35:08Z

src/sourcemaps/scanner.js

+      file.mapPathName = mapPath;
+      file.sourceMappingURL = true;
+    } else {
+      output.warn('', 'map not found');


What I meant here is that if we don't find map path do we want to prompt the user to enter the path manually but probably that won't be intuitive.

mrunalk · 2020-08-26T22:48:31Z

src/sourcemaps/scanner.js

+    await this.scanFiles();
+  }
+
+  async scanFiles() {


Is this mainly scanning for minified files and then extracting map path from those min files? If yes then can you rename this to something like scanMinifiedFiles()?

mrunalk · 2020-08-26T22:52:36Z

src/sourcemaps/command.js

+    quiet: argv['quiet']
+  });
+
+  const scanner = new Scanner({


Can this scanner adapt to dsym or other symbol files in future?

I would assume not since the source-map npm package is the standard package for Javascript source maps.

Thanks. So looks like we will have to implement separate scanner for scanning dsym files.

mrunalk · 2020-08-26T22:52:53Z

src/sourcemaps/command.js

+
+  await scanner.scan();
+
+  const uploader = new Uploader({


Can this uploader adapt to dsym or other symbol files in future?

I suppose it depends on how different Rollbar's API is for dsym, and how different the data structures are. It's not something I've evaluated.

Got it. I was just asking so that in near future if we end up adding dsym or proguard symbol upload functionality we shouldn't have to do majore rewrite of the CLI.

waltjones · 2020-08-27T14:32:37Z

@mrunalk The existing test fixtures are TypeScript (Angular build) and ES6 (React build) original sources. soucesContent key in the respective map files will confirm.

mrunalk

LGTM. Nice work @waltjones.

mrunalk · 2020-08-28T17:29:48Z

src/sourcemaps/command.js

+    quiet: argv['quiet']
+  });
+
+  const scanner = new Scanner({


Thanks. So looks like we will have to implement separate scanner for scanning dsym files.

mrunalk · 2020-08-28T17:32:24Z

src/sourcemaps/command.js

+
+  await scanner.scan();
+
+  const uploader = new Uploader({


Got it. I was just asking so that in near future if we end up adding dsym or proguard symbol upload functionality we shouldn't have to do majore rewrite of the CLI.

initial sourcemaps, upload from target dir

ae7a9ff

mrunalk self-requested a review August 19, 2020 22:13

waltjones mentioned this pull request Aug 21, 2020

Create node.js.yml (Github CI workflow) #3

Closed

waltjones force-pushed the wj-initial-sourcemaps branch from 7d9d553 to 7acafa5 Compare August 21, 2020 19:51

waltjones added 6 commits August 21, 2020 13:24

update command line to match Rollbar spec

ac4383c

restore dry-run (validate) option, as it enables better testing

05a1405

add fixtures (representative source map builds)

1367c5a

add test automation and coverage report

7ac2a07

Create node.js.yml (Github CI workflow)

ad6e49d

update readme

35ec481

waltjones force-pushed the wj-initial-sourcemaps branch from 7acafa5 to 35ec481 Compare August 21, 2020 20:27

waltjones changed the title ~~WIP: Initial sourcemaps: upload from target dir~~ Initial sourcemaps: upload from target dir Aug 21, 2020

waltjones added the Ready for review label Aug 24, 2020

vselvarajijay approved these changes Aug 25, 2020

View reviewed changes

mrunalk reviewed Aug 25, 2020

View reviewed changes

refactor validate method to return errors array

298dc52

mrunalk suggested changes Aug 26, 2020

View reviewed changes

mrunalk approved these changes Aug 28, 2020

View reviewed changes

waltjones merged commit 208ae6d into master Aug 28, 2020

waltjones mentioned this pull request Mar 29, 2021

Is there any example for Angular Source Maps Implementation? rollbar/rollbar.js#940

Closed

ajtran deleted the wj-initial-sourcemaps branch April 4, 2022 22:35

TheDevFactory mentioned this pull request Jun 24, 2025

Fix sourcemap file resolution for Next.js apps with --next parameter #24

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Initial sourcemaps: upload from target dir #1

Initial sourcemaps: upload from target dir #1

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Initial sourcemaps: upload from target dir #1

Initial sourcemaps: upload from target dir #1

Uh oh!

Conversation

Uh oh!

Description of the change

Coverage report

Fixtures

Examples

Type of change

Development

Code review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!