Use sourcemaps to revert stacktrace in extension #995

wardpeet · 2016-11-21T17:14:04Z

Should fix #972, downside is that I had to fix an issue with browserify sourcemaps inside novocaine/sourcemapped-stacktrace#19

perhaps you know of another library? (didn't want to write it myself)

brendankenny

perhaps you know of another library

I don't, but maybe @addyosmani or @paulirish (when he's back) know of one or a simpler way to do this?

brendankenny · 2016-11-21T23:46:04Z

lighthouse-extension/package.json

@@ -23,5 +23,8 @@
    "gulp-zip": "^3.2.0",
    "run-sequence": "^1.1.5",
    "through2": "^2.0.1"
+  },
+  "dependencies": {
+    "sourcemapped-stacktrace": "^1.1.3"


should be in devDependencies

i require it inside browserify doesn't that make it a dependency?

i think in practical terms it doesnt matter, but i agree that semantically it fits in deps more than devDeps

brendankenny · 2016-11-22T00:17:29Z

lighthouse-extension/app/src/popup.js

@@ -157,7 +166,10 @@ document.addEventListener('DOMContentLoaded', _ => {

      if (!multipleTabs && !debuggerExists) {
        feedbackEl.className = 'feedback-error';
-        feedbackEl.appendChild(buildReportErrorLink(err));
+        buildReportErrorLink(err)


ideally we'd want the error passed to background.console.error below to also be source mapped

brendankenny · 2016-11-22T00:18:10Z

lighthouse-extension/app/src/popup.js

-    const body = '&body=' + encodeURI(qsBody);
+    return new Promise(resolve => {
+      sourcemapStacktrace.mapStackTrace(err.stack, function(mapped) {
+        qsBody += '**Stack Trace**:\n ```' + mapped.join('\n') + '```';


will need to rebase this after #992

paulirish

don't know of a better library or simpler way to do this, myself. (unless we use devtools frontend for the same resolution, which won't be very fun IMO)

this seems generally on point tho

paulirish · 2016-11-29T00:14:36Z

lighthouse-extension/package.json

@@ -23,5 +23,8 @@
    "gulp-zip": "^3.2.0",
    "run-sequence": "^1.1.5",
    "through2": "^2.0.1"
+  },
+  "dependencies": {
+    "sourcemapped-stacktrace": "^1.1.3"


i think in practical terms it doesnt matter, but i agree that semantically it fits in deps more than devDeps

paulirish · 2016-11-29T00:16:25Z

lighthouse-extension/package.json

@@ -23,5 +23,8 @@
    "gulp-zip": "^3.2.0",
    "run-sequence": "^1.1.5",
    "through2": "^2.0.1"
+  },
+  "dependencies": {


do we need to wait for your PR to land upstream first?

if necessary I can publish the github repo under my name in npm repo until the pr is accepted

@paulirish PR got accepted ^^

in that case you'll want to bump to 1.1.4 ;)

paulirish · 2016-11-29T00:24:20Z

lighthouse-extension/app/src/popup.js

@@ -122,6 +126,21 @@ document.addEventListener('DOMContentLoaded', _ => {
    return listItem;
  }

+  function showError(err, message, includeReportLink) {
+    sourcemapStacktrace.mapStackTrace(err.stack, stackWithSourcemaps => {


can we do a try/catch around the mapStackTrace call? I want to make sure we gracefully degrade and still show the error reporting stuff even if the sourcemap resolution fails.

Need to test my new approach :)

wardpeet · 2016-12-02T21:14:32Z

Background.js looks like this

Issue looks like this

paulirish · 2016-12-02T21:55:45Z

Tried it out when I was hitting some bugs. Some examples of this PR in action:

before and after - exception 1

before and after - exception 2

paulirish · 2016-12-02T22:15:25Z

I like the added benefit this PR gives our error stack traces. The impact on our bundle size for adding this new module is ~90kb which is similar to the jszip that we removed.

However on the other hand, this means including sourcemaps for our entire bundle which takes the filesize from 2.4mb to 6.5mb.

wardpeet · 2016-12-03T13:08:23Z

mmh I don't think we can fix the sourcemap issue but I might be able to pick relevant pieces of the library and put them in lighthouse but probably won't save that much because I will still be dependent on the sourcemap dependency

wardpeet · 2016-12-09T23:57:46Z

@paulirish don't forget we are also removing url and whatwg-url from the extension.

wardpeet · 2016-12-20T21:54:07Z

@paulirish I did a new check with a rebase of master and got the following results
Master is still 2.4MB
My branch is 6,0MB

Is it that bad? We could also do a better issue tracking and use the report viewer to convert stacktraces so the extension stays 2MB and the heavy lifting is done by the viewer but than we need to build a small ui for it.

brendankenny · 2017-01-12T22:47:07Z

should we close this? Looks like the impact on code size would be too great? It seems like it hasn't been too hard to backtrack from extension strack traces to original source code

wardpeet · 2017-01-13T17:36:20Z

@brendankenny yeah think closing is ok :)

wardpeet added chillin ❄ extension labels Nov 21, 2016

brendankenny requested changes Nov 22, 2016

View reviewed changes

wardpeet force-pushed the revert-stacktrace branch from d94a610 to 3917aba Compare November 24, 2016 20:23

paulirish reviewed Nov 29, 2016

View reviewed changes

wardpeet force-pushed the revert-stacktrace branch from 3917aba to f64ef53 Compare December 2, 2016 20:41

wardpeet force-pushed the revert-stacktrace branch from e31080f to c9137d7 Compare December 9, 2016 23:58

wardpeet added 6 commits December 20, 2016 22:48

Add extension label to issue

1a166fc

Convert stacktrace with sourcemaps to have better error messages

b6f98b8

rebase from master

19ce4ff

Try Catch sourcemapStacktrace

bc3617b

Updated stacktrace version

b3d56b0

Fix sourcemap reporting

f296a22

wardpeet force-pushed the revert-stacktrace branch from c9137d7 to f296a22 Compare December 20, 2016 21:54

wardpeet closed this Jan 16, 2017

brendankenny mentioned this pull request Jan 23, 2017

Create and ship source map for better extension stack traces #972

Closed

wardpeet deleted the revert-stacktrace branch July 24, 2017 05:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use sourcemaps to revert stacktrace in extension #995

Use sourcemaps to revert stacktrace in extension #995

wardpeet commented Nov 21, 2016 •

edited

Loading

brendankenny left a comment

brendankenny Nov 21, 2016

wardpeet Nov 22, 2016

paulirish Nov 29, 2016

brendankenny Nov 22, 2016

wardpeet Nov 24, 2016

brendankenny Nov 22, 2016

wardpeet Nov 24, 2016

paulirish left a comment

paulirish Nov 29, 2016

paulirish Nov 29, 2016

wardpeet Nov 29, 2016

wardpeet Dec 1, 2016

paulirish Dec 1, 2016

paulirish Nov 29, 2016

wardpeet Dec 1, 2016

wardpeet commented Dec 2, 2016

paulirish commented Dec 2, 2016

paulirish commented Dec 2, 2016

wardpeet commented Dec 3, 2016

wardpeet commented Dec 9, 2016

wardpeet commented Dec 20, 2016

brendankenny commented Jan 12, 2017

wardpeet commented Jan 13, 2017

Use sourcemaps to revert stacktrace in extension #995

Use sourcemaps to revert stacktrace in extension #995

Conversation

wardpeet commented Nov 21, 2016 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wardpeet commented Dec 2, 2016

paulirish commented Dec 2, 2016

before and after - exception 1

before and after - exception 2

paulirish commented Dec 2, 2016

wardpeet commented Dec 3, 2016

wardpeet commented Dec 9, 2016

wardpeet commented Dec 20, 2016

brendankenny commented Jan 12, 2017

wardpeet commented Jan 13, 2017

wardpeet commented Nov 21, 2016 •

edited

Loading