Use sorcery to merge sourcemaps between browserify and gulp-typescript by weswigham · Pull Request #9439 · microsoft/TypeScript

weswigham · 2016-06-30T10:38:17Z

Yesterday @sandersn and @Andy-MS both mentioned that their sourcemaps for in-browser debugging seemed off. I checked, and despite loading in the correct files, the maps were indeed off by around 1 line in most places! After looking into it, I determined it was because we were never integrating any sourcemaps from the browserify transform step of the browser tests build. Now, browserify doesn't ingest sourcemaps from the incoming stream particularly well (or at all), so I extracted the single-stage sourcemaps it makes and merged two two layers of sourcemaps using a library called sorcery. I've tried combining them using vinyl-sourcemaps-apply and combine-source-maps (more popular libraries), but it seems like those libraries are focused more for concatenation, not redirection/layering - for some reason neither of them actually merge both the actual sourcemaps and source files list correctly (whereas sorcery does, with a little coercion).

@rbuckton You typically have opinions on all things gulp - do you have any suggestions/feedback here?

mhegazy · 2016-06-30T19:48:07Z

+}
+
+declare module "sorcery" {
+    export = any;


is there a value any defined any where?

Might have intended declare module "sorcery";

@Andy-MS Ah, I had to install a new nightly build to use declare module "sorcery"; - for some reason I thought I needed to wait for an LKG. I'll change it.

@mhegazy ...no? (Not that I know of, anyway) Is this not supposed to work? Because it does.

sandersn · 2016-07-07T22:21:48Z

-            browserify(intoStream(file.contents))
+            const originalMap = file.sourceMap;
+            const prebundledContent = file.contents.toString();
+            originalMap.sources = originalMap.sources.map(s => path.resolve(s)); // Make paths absolute to help sorcery deal with all the terrible paths being thrown around


nit: ...map(path.resolve) should work fine.

Move comments above

Scratch that, map(path.resolve) throws with perhaps the oddest assertion I've ever seen. Comments still moved.

DanielRosenwasser · 2016-07-07T22:24:52Z

-                    file.contents = res;
+                    const maps = JSON.parse(convertMap.fromSource(res.toString(), /*largeSource*/true).toJSON());
+                    delete maps.sourceRoot;
+                    for (let i = 0; i < maps.sources.length; i++) {


Need the indexes to do the assignments. I could rewrite it as a map, however, and will do so.

sandersn · 2016-07-07T22:56:18Z

-                    file.contents = res;
+                    let maps = JSON.parse(convertMap.fromSource(res.toString(), /*largeSource*/true).toJSON());
+                    delete maps.sourceRoot;
+                    maps = maps.map(s => (s === "_stream_0.js") ? path.resolve("built/local/_stream_0.js") : path.resolve(s));


push the conditional down into the path.resolve argument

sandersn · 2016-07-07T23:16:11Z

👍 -- works for me. 1 minor nit about the conditional left.

sandersn · 2016-07-08T16:15:11Z

Well, actually it needs a gulp clean whenever I change the source. Is this task run whenever runtests-parallel runs and the source has changed? It doesn't act like it is.

rbuckton · 2016-07-08T16:19:04Z

I sent an email to @jrieken about the feasibility of using gulp-tsb (which is used by VS Code) instead of gulp-typescript, though it needs some updates to work well for us. One of the changes I made in the referenced PR should make sourcemaps much cleaner.

sandersn · 2016-07-08T16:27:43Z

Would the incrementality of gulp-tsb fix the fact that this task doesn't update source maps when source code changes?

rbuckton · 2016-07-08T16:30:05Z

I'm not certain. I'm testing my fork of gulp-tsb right now. Do you have simplified repro steps I could use to check this scenario?

sandersn · 2016-07-08T16:36:49Z

Edit: here is a reusable repro. I don't have any idea how to automate this, but maybe it will still be useful:

gulp runtests-parallel --t=contextuallyTypedObjectLiteral --browser=chrome
Put a breakpoint on a line in checker.ts:checkExpressionCached (line 12880 is good on 2fb2a4b if that's what you're starting from).
Run the debugger and confirm that the breakpoint hits.
Step a few times and confirm that the debugger steps correctly.
Stop the debugger.
Add a few console.log calls earlier in the file (line 63 is fine, but anything above line 10,000 or so should be fine).
Repeat steps 1-5. Step 3 or 4 will fail this time. If step 4 fails, you will specifically see that the debugger is offset by the number of added lines.
gulp clean
Repeat steps 1-5. They should now succeed.

rbuckton · 2016-07-10T09:16:50Z

So it seems there are various issues with source maps across gulp-typescript, gulp-sourcemaps, and browserify. I've done some investigation and it boils down into a few main themes:

gulp-sourcemaps generally seems to expect that the file that the source map is attached to is effectively the same vinyl file as the original source, just with its contents modified. As a result, it expects the source map itself to be created as if it were adjacent to the sources (not the outputs).
gulp-sourcemaps does not respect the "sourceRoot" property of an output source map.
gulp-typescript does some odd things with paths when it generates the source map. In investigating gulp-tsb, I had to avoid those pitfalls and try to make the paths the same as what gulp-sourcemaps expects.
browserify doesn't respect existing sourcemaps, and the sourcemap it generates doesn't align with the requirements of gulp-sourcemaps.

I've experimented with using my fork of gulp-tsb in place of gulp-typescript, and was able to use gulp-sourcemaps-apply to affect something like what @weswigham is doing with sorcery:

import rename = require("gulp-rename");
import source = require("vinyl-source-stream");
import buffer = require("vinyl-buffer");
import applySourceMaps = require("vinyl-sourcemaps-apply");
...
gulp.task("browserify", "Runs browserify on run.js to produce a file suitable for running tests in the browser", [run], () => {
    const b = browserify("./run.js", { debug: true, basedir: path.resolve(builtLocalDirectory) });
    return b.bundle()
        .pipe(source("run.js", path.resolve(harnessDirectory))) // emulate a file in src/harness
        .pipe(buffer())
        .pipe(sourcemaps.init({ loadMaps: true })) // pull in the inline sourcemap from browserify
        .pipe(through2.obj((bundle, enc, cb) => {
            gulp.src(run)
                .pipe(sourcemaps.init({ loadMaps: true }))
                .on("data", file => {
                    // truncate source root and repoint source maps
                    delete file.sourceMap.sourceRoot; 
                    delete bundle.sourceMap.sourceRoot;
                    const sourceMap = bundle.sourceMap;
                    bundle.sourceMap = file.sourceMap;
                    applySourceMaps(bundle, sourceMap);
                    cb(null, bundle);
                });
        }))
        .pipe(rename("bundle.js"))
        .pipe(sourcemaps.write(".", { includeContent: false, destPath: builtLocalDirectory })) // sets the correct sourceRoot
        .pipe(gulp.dest(builtLocalDirectory));
});

I still have some testing to do to make sure it is working as expected, however.

sandersn · 2016-07-13T21:43:11Z

@rbuckton, are you OK with @weswigham merging this now and implementing the right fix later? I have continued to use this branch and have not had any problems, but it would be way more convenient to have it in master.

rbuckton · 2016-07-14T02:33:10Z

After addressing the merge conflicts, 👍

Use sorcery to merge sourcemaps between browserify and gulp-typescript

a244f9f

msftclas added the cla-already-signed label Jun 30, 2016

mhegazy reviewed Jun 30, 2016
View reviewed changes

Use shorthand

07aa17f

sandersn reviewed Jul 7, 2016
View reviewed changes

Fix nit

b967c75

DanielRosenwasser reviewed Jul 7, 2016
View reviewed changes

weswigham added 2 commits July 7, 2016 15:28

move comments, change loop into map

5ae902f

Merge branch 'master' into merge-browserify-sourcemaps

6c10782

sandersn reviewed Jul 7, 2016
View reviewed changes

Ahahaha, we should run code before pushing it

ace6bf5

Move conditional into call

2fb2a4b

Merge branch 'master' into merge-browserify-sourcemaps

15aa4d6

weswigham merged commit 761482c into microsoft:master Jul 15, 2016

weswigham deleted the merge-browserify-sourcemaps branch July 15, 2016 23:55

microsoft locked and limited conversation to collaborators Jun 19, 2018

Conversation

weswigham commented Jun 30, 2016

Uh oh!

mhegazy Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

ghost Jun 30, 2016

Choose a reason for hiding this comment

Uh oh!

weswigham Jun 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandersn Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DanielRosenwasser Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

weswigham Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn Jul 7, 2016

Choose a reason for hiding this comment

Uh oh!

sandersn commented Jul 7, 2016

Uh oh!

sandersn commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sandersn commented Jul 8, 2016

Uh oh!

rbuckton commented Jul 8, 2016

Uh oh!

sandersn commented Jul 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rbuckton commented Jul 10, 2016

Uh oh!

sandersn commented Jul 13, 2016

Uh oh!

rbuckton commented Jul 14, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

weswigham Jun 30, 2016 •

edited

Loading

weswigham Jul 7, 2016 •

edited

Loading

sandersn commented Jul 8, 2016 •

edited

Loading

rbuckton commented Jul 8, 2016 •

edited

Loading

sandersn commented Jul 8, 2016 •

edited

Loading