Wrap the newest version of 'source-map'. (issue 7789051)

[usrb...@yahoo.com]

Reviewers: arv-chromium, slightlylate, johnjbarton,

Message:
Note: This is more of an RFC at this point, because we should really
fix the upstream semicolon bugs before integrating the new source-map.
But this does bring up the issue of traceur always being in danger of
being broken by a dependency update.

----

I actually have three implementations:
- Patch Set 1 is the simplest,
- Patch Set 2 is slightly more complex (but cleaner, I think), and
- the third one handles relative paths between modules, and is overkill
for the task of wrapping source-map. I ended up not including that
because there's a lot of code that we may never exercise, and so bugs
may stay there for a long time.

See the last (non-comment) section for a demo.

I originally considered generating the include directives, but if you
want to do the entire thing (including deciding which files to include
in the first place) automatically, then you really have to have much
smarter code, and you may really have to do a test load of a module in
order to find out the true dependencies.

// this can probably be statically analyzed, but I'm sure it gets
// complicated fast.
var modules = ['module1', 'module2'].map(require);

Too big of a project for the simple goal of wrapping a single library,
so I stopped there.

----

Here is a quick demo of the third implementation mentioned earlier:

#--cut--
cat > require.js <<"END"
'use strict';

/**
* @param {string} b Base path to add to module relative paths. Assumed
to be
* normalized.
* @param {string} p Path to normalize
* @return {string} A path normalized according to normal rules, except
that
* paths starting with dot segments always retain at least one dot
segment.
* This is so that module relative paths remain relative, and module
* top-level paths remain top-level.
*/
function normalize(b, p) {
var pi = p.split('/');
var po = [];
var segs = 0;

function addSeg(seg) {
switch (seg) {
case '..':
if (segs) {
segs--;
if (po.pop() !== '.')
return;
}
po.push('..');
return;
case '.':
if (!po.length) {
segs++;
po.push('.');
}
return;
default:
if (po.length) {
segs++;
}
po.push(seg);
}
}

if (pi[0] === '.' || pi[0] === '..') {
b.split('/').forEach(addSeg);
}
p.split('/').forEach(addSeg);
return po.join('/');
}

/**
* @param {object} mapping Maps between module ids and exports.
* @param {string} id A module id.
* @return {function} a 'define' function that initializes |mapping[id]|
with
* the exports from |factory|.
*/
function makeDefine(mapping, id) {
return function(factory) {
mapping[id] = factory;
};
}

function makeRequire(mapping) {
var bases = ['.'];
function resolve(id) {
if (!/\.{0,2}\//.exec(id))
return id;
var base = bases[bases.length - 1];
return normalize(base, id);
}
function dirname(id) {
if (!/\//.test(id)) {
return id;
}
return /(.*)(?:\/.*)$/.exec(id)[1];
}
function require(id) {
var factory, exports, module = {};
switch (typeof mapping[id = resolve(id)]) {
case 'undefined':
throw new TypeError(id + ' not defined.');
case 'function':
console.log('id:%s', id);
factory = mapping[id];
exports = mapping[id] = {};
bases.push(dirname(id));
factory(require, exports, module);
bases.pop();
return exports;
case 'object':
return mapping[id];
}
}
return require;
}

var mapping = {};
var define, require = makeRequire(mapping);
define = makeDefine(mapping, 'hello');
define(function(r,exports,c){
var world = require('./dir/world').world;
exports.hello = 'hello ' + world;
exports.hola = require('./hola').hola;
})
define = makeDefine(mapping, 'hello/hola');
define(function(r,exports,c){
exports.hola = 'hola';
})
define = makeDefine(mapping, 'hello/dir/world');
define(function(r,exports,c){
var rld = require('./rld').rld;
exports.world = 'wo' + rld;
})
define = makeDefine(mapping, 'hello/dir/rld');
define(function(r,exports,c){
exports.rld = 'rld';
})
define = makeDefine(mapping, './hwdir/hw');
define(function(r,exports,c){
exports.hw = require('hello');
exports.jo = require('../jo').jo;
})
define = makeDefine(mapping, './jo');
define(function(r,exports,c){
exports.jo = 'jo';
})
console.log(require('./hwdir/hw'));
END

node require.js
#--cut--



https://codereview.appspot.com/7789051/diff/2001/Makefile
File Makefile (left):

https://codereview.appspot.com/7789051/diff/2001/Makefile#oldcode11
Makefile:11: TFLAGS = --strict-semicolons --
The newest version of source-map (0.1.16 currently) has some missing
semicolons, so I've temporarily taken out this flag in order to allow
the build to complete successfully.

Description:
Wrap the newest version of 'source-map'.

- Add 'source-map' to package.json

BUG=https://code.google.com/p/traceur-compiler/issues/detail?id=222
TEST=None

Please review this at https://codereview.appspot.com/7789051/

Affected files:
M Makefile
M package.json
M src/outputgeneration/SourceMapIntegration.js-template.js


Index: Makefile
diff --git a/Makefile b/Makefile
index
70fabfadf7e21946ee9584806724be4154c987de..201794b3ce1bba8851adc8708125bece339a45ff
100644
--- a/Makefile
+++ b/Makefile
@@ -8,7 +8,7 @@ GENSRC = \
src/syntax/trees/ParseTrees.js \
src/syntax/ParseTreeVisitor.js

-TFLAGS = --strict-semicolons --
+TFLAGS = --

build: bin/traceur.js

Index: package.json
diff --git a/package.json b/package.json
index
465fc4570f530881242eb52650d37afb65dcadbe..08880f7caa335cbd93491c72cf065d4a80277a27
100644
--- a/package.json
+++ b/package.json
@@ -35,6 +35,7 @@
"commander": ">=1.1"
},
"devDependencies": {
- "closure-library": ">=1.0"
+ "closure-library": ">=1.0",
+ "source-map": ">=0.1.16"
}
}
Index: src/outputgeneration/SourceMapIntegration.js-template.js
diff --git a/src/outputgeneration/SourceMapIntegration.js-template.js
b/src/outputgeneration/SourceMapIntegration.js-template.js
index
8ae6fe705702f2a32a74028387733e4eea452b89..2c59b8163375ac25c28817a60d45106607a28a8f
100644
--- a/src/outputgeneration/SourceMapIntegration.js-template.js
+++ b/src/outputgeneration/SourceMapIntegration.js-template.js
@@ -12,34 +12,61 @@
// See the License for the specific language governing permissions and
// limitations under the License.

-var moduleExports = {};
+// 'source-map' (>=0.1.16) requires a 'define' function that matches the
+// CommonJS module specification. Since it only uses a subset of that
+// functionality, we cheat here and only provide that minimum subset.

-// 'source-map' changes its loading behavior depending on what
the 'exports'
-// and 'define' symbols refer to.
-//
-// For the behavior we want, we set up the environment such that:
-//
-// exports === undefined
-// define === undefined
-// this === moduleExports // Receives the 'sourceMapModule' export.
+/**
+ * @param {object} mapping Maps between module ids and exports.
+ * @param {string} id A module id.
+ * @return {function} a 'define' function that initializes |mapping[id]|
with
+ * the exports from |factory|.
+ */
+function makeDefine(mapping, id) {
+ return function(factory) {
+ mapping[id] = factory;
+ };
+}
+
+function makeRequire(mapping) {
+ function require(id) {
+ var factory, exports, module = {};
+ switch (typeof mapping[id]) {
+ case 'undefined':
+ throw new TypeError(id + ' not defined.');
+ case 'function':
+ factory = mapping[id];
+ exports = mapping[id] = {};
+ factory(require, exports, module);
+ return exports;
+ case 'object':
+ return mapping[id];
+ }
+ }
+ return require;
+}

-(function(exports, define) {
-// #include ../../third_party/source-map/lib/source-map/array-set.js
-// #include ../../third_party/source-map/lib/source-map/base64.js
-// #include ../../third_party/source-map/lib/source-map/base64-vlq.js
-// #include ../../third_party/source-map/lib/source-map/binary-search.js
-// #include ../../third_party/source-map/lib/source-map/util.js
-//
#include ../../third_party/source-map/lib/source-map/source-map-generator.js
-//
#include ../../third_party/source-map/lib/source-map/source-map-consumer.js
-// #include ../../third_party/source-map/lib/source-map/source-node.js
-}).call(moduleExports);
+var mapping = {};
+var define, require = makeRequire(mapping);

-var sourceMapModule = moduleExports.sourceMapModule;
+define = makeDefine(mapping, './util');
+// #include ../../node_modules/source-map/lib/source-map/util.js
+define = makeDefine(mapping, './array-set');
+// #include ../../node_modules/source-map/lib/source-map/array-set.js
+define = makeDefine(mapping, './base64');
+// #include ../../node_modules/source-map/lib/source-map/base64.js
+define = makeDefine(mapping, './base64-vlq');
+// #include ../../node_modules/source-map/lib/source-map/base64-vlq.js
+define = makeDefine(mapping, './binary-search');
+// #include ../../node_modules/source-map/lib/source-map/binary-search.js
+define = makeDefine(mapping, './source-map-generator');
+//
#include ../../node_modules/source-map/lib/source-map/source-map-generator.js
+define = makeDefine(mapping, './source-map-consumer');
+//
#include ../../node_modules/source-map/lib/source-map/source-map-consumer.js
+define = makeDefine(mapping, './source-node');
+// #include ../../node_modules/source-map/lib/source-map/source-node.js

-export var base64 = sourceMapModule['base64'];
-export var base64Vlq = sourceMapModule['base64-vlq'];
-export var binarySearch = sourceMapModule['binary-search'];
-export var util = sourceMapModule['util'];
-export var SourceMapGenerator = sourceMapModule['source-map-generator'];
-export var SourceMapConsumer = sourceMapModule['source-map-consumer'];
-export var SourceNode = sourceMapModule['source-node'];
+export var
+ SourceMapGenerator =
require('./source-map-generator').SourceMapGenerator,
+ SourceMapConsumer = require('./source-map-consumer').SourceMapConsumer,
+ SourceNode = require('./source-node').SourceNode;

[a...@chromium.org]

I prefer the simpler approach in patch set 1.

I don't want us to go all crazy on AMD at this point.


https://codereview.appspot.com/7789051/diff/1/src/outputgeneration/SourceMapIntegration.js-template.js
File src/outputgeneration/SourceMapIntegration.js-template.js (right):

https://codereview.appspot.com/7789051/diff/1/src/outputgeneration/SourceMapIntegration.js-template.js#newcode20
src/outputgeneration/SourceMapIntegration.js-template.js:20: * @param

{object} mapping Maps between module ids and exports.

{Object}

https://codereview.appspot.com/7789051/diff/1/src/outputgeneration/SourceMapIntegration.js-template.js#newcode22
src/outputgeneration/SourceMapIntegration.js-template.js:22: * @return

{function} a 'define' function that initializes |mapping[id]| with

{Function}

or

{function(function(T, V, W))}

https://codereview.appspot.com/7789051/diff/1/src/outputgeneration/SourceMapIntegration.js-template.js#newcode54
src/outputgeneration/SourceMapIntegration.js-template.js:54:
SourceMapGenerator =
mapping['./source-map-generator'].SourceMapGenerator,
This indentation is strange. I think the following reads better:

export var SourceMapGenerator
mapping['./source-map-generator'].SourceMapGenerator;
export var SourceMapConsumer =
mapping['./source-map-consumer'].SourceMapConsumer;
export var SourceNode = mapping['./source-node'].SourceNode;

https://codereview.appspot.com/7789051/

[usrb...@yahoo.com]

Switched back to the simpler implementation of Patch Set 1.

----

I sent a pull request fixing the missing semicolons, and it got
committed 2 minutes later. Maybe that's not a record, but it has to be
pretty close. I was impressed, at least. Not sure when the fixed version
will be up on npmjs.org, but it shouldn't be that long.

----

"nolint" is probably the way to go eventually. The easy way is to use
magic strings similar to "use strict"; the harder way is to reconfigure
the scanner to emit comment tokens.



https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js
File src/outputgeneration/SourceMapIntegration.js-template.js (right):

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode15
src/outputgeneration/SourceMapIntegration.js-template.js:15: // The
official 'source-map' requires a 'define' function that matches the
I checked through the official repo's history, and noticed that they've
always used the 'define' interface. It turns out that only our version
in 'third_party/source-map' has the extra machinery. It's kind of like
when you suddenly realize there is no Santa Claus. Or something like
that.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode22
src/outputgeneration/SourceMapIntegration.js-template.js:22: * @return
{function(function(require, exports, module))} a 'define' function
I tried using the official param names (unless the T, V, W is an
established convention); the longer JSDoc does sort of impact
readability considering you don't have syntax highlighting.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode28
src/outputgeneration/SourceMapIntegration.js-template.js:28: var module
= null; // Unused arg. Included for completeness.
Micro-optimization: don't create an object that's never used.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode56
src/outputgeneration/SourceMapIntegration.js-template.js:56: export var
base64Vlq = mapping['./base64-vlq'];
I ended up adding all the exports back, because sometimes I use
base64Vlq to encode / decode a few things, and who knows when someone
might have a need to access the other exports for either testing or
debugging.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode59
src/outputgeneration/SourceMapIntegration.js-template.js:59:
mapping['./source-map-generator'].SourceMapGenerator;
I kind of wish we wrapped at 90 or 100 sometimes. Slowly getting used to
this style though.

https://codereview.appspot.com/7789051/

[a...@chromium.org]

LGTM

I'll take care of my comments before submitting

https://codereview.appspot.com/7789051/diff/2001/Makefile
File Makefile (left):

https://codereview.appspot.com/7789051/diff/2001/Makefile#oldcode11
Makefile:11: TFLAGS = --strict-semicolons --

On 2013/03/23 21:21:51, usrbincc wrote:
> The newest version of source-map (0.1.16 currently) has some missing
> semicolons, so I've temporarily taken out this flag in order to allow
> the build to complete successfully.

I think we will continue to run into this as long as we include third
party code.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js
File src/outputgeneration/SourceMapIntegration.js-template.js (right):

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode22
src/outputgeneration/SourceMapIntegration.js-template.js:22: * @return
{function(function(require, exports, module))} a 'define' function

On 2013/03/24 19:17:36, usrbincc wrote:
> I tried using the official param names (unless the T, V, W is an
> established convention); the longer JSDoc does sort of impact
> readability considering you don't have syntax highlighting.

Those should be the type names...

Function is for all types

function(T, V) : W is for a function that takes a T and V and returns a
W. I don't think the type info here is important so I would just go with
Function.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode56
src/outputgeneration/SourceMapIntegration.js-template.js:56: export var
base64Vlq = mapping['./base64-vlq'];

On 2013/03/24 19:17:36, usrbincc wrote:
> I ended up adding all the exports back, because sometimes I use
> base64Vlq to encode / decode a few things, and who knows when someone
> might have a need to access the other exports for either testing or
> debugging.

I would prefer us to not export things we don't need.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode59
src/outputgeneration/SourceMapIntegration.js-template.js:59:
mapping['./source-map-generator'].SourceMapGenerator;

On 2013/03/24 19:17:36, usrbincc wrote:
> I kind of wish we wrapped at 90 or 100 sometimes. Slowly getting used
to
> this style though.

Working in WebKit code has made me believe that there are cases where
line limits are stupid. Import and exports seems like those.


We could also do m instead of mapping ;-)

https://codereview.appspot.com/7789051/

[a...@chromium.org]

Committed as 2070a8aeb6406db06b847c0e7fca6934504dc4fa

Please take a look at the changes I made.

1. Fixed the type annotation
2. Only export SourceMapGenerator, SourceMapConsumer, SourceNode
3. Removed third_party/source-map

I tested and it all works (needs npm install)

https://codereview.appspot.com/7789051/

[usrb...@yahoo.com]

> 2. Only export SourceMapGenerator, SourceMapConsumer, SourceNode

That's probably the right thing to do. Cleaner-looking too. Upon
reflection, I can always require() directly if I want to test something
quick.

https://codereview.appspot.com/7789051/diff/2001/Makefile
File Makefile (left):

https://codereview.appspot.com/7789051/diff/2001/Makefile#oldcode11
Makefile:11: TFLAGS = --strict-semicolons --

On 2013/03/25 13:51:05, arv-chromium wrote:
> On 2013/03/23 21:21:51, usrbincc wrote:
> > The newest version of source-map (0.1.16 currently) has some missing
> > semicolons, so I've temporarily taken out this flag in order to
allow
> > the build to complete successfully.

> I think we will continue to run into this as long as we include third
party
> code.

I plan on submitting a "nolint" patch in the near future, so this
'--strict-semicolons' removal should only be a temporary thing.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js
File src/outputgeneration/SourceMapIntegration.js-template.js (right):

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode22
src/outputgeneration/SourceMapIntegration.js-template.js:22: * @return
{function(function(require, exports, module))} a 'define' function

> Those should be the type names...

Oops. That's the trouble with a mentally-simulated type language. Coding
is so much easier with computer support.

https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceMapIntegration.js-template.js#newcode59
src/outputgeneration/SourceMapIntegration.js-template.js:59:
mapping['./source-map-generator'].SourceMapGenerator;

> Working in WebKit code has made me believe that there are cases where
line
> limits are stupid. Import and exports seems like those.

I think that's one area that's harder to apply the Google JS style guide
to (understandable considering JS doesn't have modules). I recently
added some exceptions to my pre-commit hook to optionally ignore module-
related lines.

I like having some sort of line limit, since that allows you to edit two
(or even three) files simultaneously in a side-by-side format.

The TypeScript coding style has no line limits, which works well most of
the time, since most lines don't go over 100, or even 90 for that
matter. The super-long lines (200+) in certain files are kind of
annoying, though. Anything that wraps is annoying, really.

#--cut--
## run in TypeScript working dir
find src -name \*.ts | xargs \
awk 'function fmt(l,n) {
if (length(l)>n)
return substr(l,1,n)"\n"fmt(substr(l,n+1),n);
return l;
}
BEGIN{ll=0;f=""}
length($0)>ll{ll=length($0);f=FILENAME;l=$0}
END{print ll,f,"\n"fmt(l,70)}'
## output; this file is a long-line festival.
411 src/services/formatting/rules.ts
this.SpaceAfterCertainKeywords = new Rule(RuleDescriptor.c
reate4(Shared.TokenRange.FromTokens([AuthorTokenKind.atkVar, AuthorTok
enKind.atkThrow, AuthorTokenKind.atkNew, AuthorTokenKind.atkDelete, Au
thorTokenKind.atkReturn, AuthorTokenKind.atkVoid, AuthorTokenKind.atkT
ypeof]), Shared.TokenRange.Any), RuleOperation.create2(new RuleOperati
onContext(Rules.IsSameLineTokenContext), RuleAction.Space));
#--cut--

With the identifier names that I gravitate to, I usually have no problem
fitting into 80 cols, but when thingsStartCamelCasing you quickly run
out of space on the right. A line length that can fit 5 to 7 average
length identifiers (including all the supporting keywords and tokens --
I'm looking at you, Mr. public static final crazyLongTypeName!) seems
about right.

Average identifier length for a given code base, obviously, since that
varies with coding style.

> We could also do m instead of mapping ;-)

I didn't think you'd actually do it! But after seeing 2070a8aeb6406db0,
I see that you've awakened to the charm of single chars. One line
squeaked by with one char of leeway. Well played. Fitting under line
length can feel like golf (wait, horseshoes? chicken?), almost.

https://codereview.appspot.com/7789051/

[Erik Arvidsson]

My personal preference is 80 or 100 line length but allow exceptions.
The problem with exceptions is that it is hard for a computer to know
when it is reasonable.

I like short variables when the context is very limited or if the
context is not important. The WebKit coding style is to name variables
in such a way that there is no need for any comment describing it
which I think is a good advice.

--
erik