Support inline source maps #82

crisptrutski · 2015-02-18T20:19:35Z

Closes #74

guybedford · 2015-02-18T20:21:25Z

lib/builder.js

    var sourceMapFile = path.basename(opts.outFile) + '.map';
    output.source += '\n//# sourceMappingURL=' + sourceMapFile;
  }

  return asp(mkdirp)(path.dirname(path.resolve(basePath, opts.outFile)))
  .then(function() {
-    if (!opts.sourceMaps) return;
+    if (!output.SourceMap) return;


crisptrutski · 2015-02-18T20:22:20Z

Having some race conditions with Cannot call method '_nodeRequire' of undefined when running tests now (via JSX plugin). Settles if running with --watch.

Would love to squish this - something I can block on to know that systemjs is ready?

guybedford · 2015-02-18T20:25:46Z

That would be if global.System is undefined. This is still needed unfortunately.

crisptrutski · 2015-02-18T20:51:05Z

Travis fails are just that issue, going see about putting in a diabolical spin loop.

guybedford · 2015-02-19T09:14:07Z

I may have fixed this build issue in 75e8eec.

guybedford · 2015-02-19T09:18:07Z

test/expectations/inline-source-map.js

+
+
+})();
+//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRyZWUvYW1kLTIuanMiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6Ijs7Ozs7T0FPTyxFQUFFLEdBQUUsQ0FBRyxJQUFFLENBQUU7RUFBRTtBQUFBIn0=


I'm worried about all these exact source tests - everytime we update compiler implementations we will be needing to update and generate these tests. How do we update the generated code? etc etc.

Sure, it's brittle as hell. Big advantage when test coverage is low is that you don't miss details. Updating the generated code could be as easy as setting an environment variable before running the tests again.

Had pretty good experience with "test everything, even the incidental things" as the initial strategy. You can immediately refactor quickly, and it's hard to write targeted tests without refactoring or abusing mocks and spies (which is often more expensive than no tests..)

Could replace this full-text match with a few assertions and keep high confidence:

Only one //# sourceMappingURL comment, and it's on the last line

Type is data:application/json;base64

Decoding and parsing it generates a valid source map

crisptrutski · 2015-02-20T11:28:34Z

@guybedford merge?

guybedford · 2015-02-20T13:25:33Z

package.json

@@ -22,7 +22,8 @@
    "es6-module-loader": "0.14.0",
    "mocha": "~2.0.0",
    "chai": "^1.10.0",
-    "react-tools": "^0.12.1"
+    "react-tools": "^0.12.1",
+    "atob": "^1.1.2"


Can we not just use Buffer directly here? https://github.com/node-browser-compat/atob/blob/master/index.js#L5

Are you sure we want to get on the wrong side of the Modularity Wars?!

Thanks, will drop atob.

crisptrutski · 2015-02-20T14:22:38Z

Hit all the feedback I think

guybedford · 2015-02-20T14:28:55Z

index.js

+  var opts = {};
+  keys.forEach(function(key) {
+    opts[key] = opts_ && opts_[key];
+  });


The point of this is to allow type inference through the setting of default values.

A common pattern I find useful is:

function prepend(a, b) { for (var p in b) { if (!(p in a)) a[p] = b[p]; } return a; } function processOpts(opts_, outFile) { // apply defaults prepend(opts_, { config: {}, lowResSourceMaps: true, minify: true, normalize: false, outFile: outFile || '', runtime: false, sourceMaps: true, sourceMapContents: _opts.sourceMaps == 'inline' }); }

Sorry to be a pain...

Almost added it as extend(defaults, normalize(opts), overrides), but then stopped because really config should also be normalized, and that triggered my subscription to the religion that configuration should never be nested, so then I went back to "smallest thing that meets this nitpickers explicit points" 👹

Sure we could add this prepend form as a "stopgap" ;)

Current behaviour has every default or falsey. Assume your example was just to demonstrate, and you're not wanting to enforce new defaults?

Ah of course - yes defaults can stay falsey.

guybedford · 2015-02-20T14:31:29Z

Looks great, thanks.

crisptrutski · 2015-02-20T15:03:11Z

OK, gave you something similar to your prepend case, but without mutating the input argument. Think it's as good enough as we can get without defining a bunch of helpers to give JS basic data wrangling powers 🔋

(what the hell.. no lambda or wizard emoji when I needed them there)

guybedford · 2015-02-20T15:06:42Z

Nice, thanks! In terms of giving JS basic data wrangling powers, Rick Waldron kindly proposed this for us in ES6 :) The pattern going forward for this in JS will be Object.assign (http://www.2ality.com/2014/12/es6-oop.html).

guybedford · 2015-02-20T15:08:18Z

Can we rebase? Then good to merge!

* Also ensure content option gets to where it needs to go

* Fix typos around (opts|output).sourceMap(s) * Fix typos around opts.lowRes(olution)SourceMaps * Fix typos around inlineSourceMap(s) * Make exports.inlineSourceMap more visible * Explicitly list all `opts` keys in one place * Remove atob dependency

crisptrutski · 2015-02-20T15:19:40Z

Rebased!

Don't link me to articles about OO, I was having such a nice day ☁️

Support inline source maps

guybedford reviewed Feb 18, 2015
View reviewed changes

crisptrutski force-pushed the inline-source-maps branch from 78e4747 to 2e5b464 Compare February 18, 2015 20:24

guybedford reviewed Feb 19, 2015
View reviewed changes

crisptrutski force-pushed the inline-source-maps branch from 3d459f6 to 296073d Compare February 20, 2015 11:00

guybedford reviewed Feb 20, 2015
View reviewed changes

crisptrutski force-pushed the inline-source-maps branch from d6994b3 to 12b2133 Compare February 20, 2015 14:21

guybedford reviewed Feb 20, 2015
View reviewed changes

crisptrutski added 9 commits February 20, 2015 17:10

Support inline source maps

c3fa85f

Fix double-source-comment for file output

23c8c4b

Extra defensive (some compiers just wanna make maps)

29fa1f0

Handle normalizing opts in one place

0f483c7

Automatically enable sourcesContent for inline maps

536500c

* Also ensure content option gets to where it needs to go

Output warning re: includeSourcesContent not being supported yet

cf4ab11

Test properties rather than exact output

a993f71

Validate base64 payload somewhat

a42772b

Fixes and refinements

2eee61e

* Fix typos around (opts|output).sourceMap(s) * Fix typos around opts.lowRes(olution)SourceMaps * Fix typos around inlineSourceMap(s) * Make exports.inlineSourceMap more visible * Explicitly list all `opts` keys in one place * Remove atob dependency

crisptrutski added 2 commits February 20, 2015 17:10

Grumblenits

0adefa0

Cast "Protection from NPE", level 1

3c90544

crisptrutski force-pushed the inline-source-maps branch from c897079 to 3c90544 Compare February 20, 2015 15:10

guybedford added a commit that referenced this pull request Feb 20, 2015

Merge pull request #82 from systemjs/inline-source-maps

ed2790c

Support inline source maps

guybedford merged commit ed2790c into master Feb 20, 2015

crisptrutski deleted the inline-source-maps branch February 20, 2015 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support inline source maps #82

Support inline source maps #82

crisptrutski commented Feb 18, 2015

guybedford Feb 18, 2015

crisptrutski Feb 18, 2015

crisptrutski commented Feb 18, 2015

guybedford commented Feb 18, 2015

crisptrutski commented Feb 18, 2015

guybedford commented Feb 19, 2015

guybedford Feb 19, 2015

crisptrutski Feb 19, 2015

crisptrutski Feb 19, 2015

crisptrutski commented Feb 20, 2015

guybedford Feb 20, 2015

crisptrutski Feb 20, 2015

crisptrutski commented Feb 20, 2015

guybedford Feb 20, 2015

crisptrutski Feb 20, 2015

crisptrutski Feb 20, 2015

guybedford Feb 20, 2015

guybedford commented Feb 20, 2015

crisptrutski commented Feb 20, 2015

guybedford commented Feb 20, 2015

guybedford commented Feb 20, 2015

crisptrutski commented Feb 20, 2015



		})();
		//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbInRyZWUvYW1kLTIuanMiXSwibmFtZXMiOltdLCJtYXBwaW5ncyI6Ijs7Ozs7T0FPTyxFQUFFLEdBQUUsQ0FBRyxJQUFFLENBQUU7RUFBRTtBQUFBIn0=

Support inline source maps #82

Support inline source maps #82

Conversation

crisptrutski commented Feb 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski commented Feb 18, 2015

guybedford commented Feb 18, 2015

crisptrutski commented Feb 18, 2015

guybedford commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski commented Feb 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski commented Feb 20, 2015

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

guybedford commented Feb 20, 2015

crisptrutski commented Feb 20, 2015

guybedford commented Feb 20, 2015

guybedford commented Feb 20, 2015

crisptrutski commented Feb 20, 2015