Issue 7789051: Wrap the newest version of 'source-map'. (Closed)
|
Created:
13 years, 2 months ago by usrbincc Modified:
13 years, 2 months ago CC:
traceur-compiler-reviews_googlegroups.com Base URL:
https://code.google.com/p/traceur-compiler/@master Visibility:
Public. |
DescriptionWrap 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
Patch Set 1 #
Total comments: 3
Patch Set 2 : Resolve dependency order automatically. #
Total comments: 3
Patch Set 3 : Use Patch Set 1 as a base. Code review fixes and minor tweaks. #
Total comments: 10
MessagesTotal messages: 7
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.
Sign in to reply to this message.
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/SourceMapI... File src/outputgeneration/SourceMapIntegration.js-template.js (right): https://codereview.appspot.com/7789051/diff/1/src/outputgeneration/SourceMapI... 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/SourceMapI... 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/SourceMapI... 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;
Sign in to reply to this message.
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/SourceM... File src/outputgeneration/SourceMapIntegration.js-template.js (right): https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceM... 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/SourceM... 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/SourceM... 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/SourceM... 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/SourceM... 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.
Sign in to reply to this message.
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/SourceM... File src/outputgeneration/SourceMapIntegration.js-template.js (right): https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceM... 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/SourceM... 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/SourceM... 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 ;-)
Sign in to reply to this message.
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)
Sign in to reply to this message.
> 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/SourceM... File src/outputgeneration/SourceMapIntegration.js-template.js (right): https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceM... 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/SourceM... 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.
Sign in to reply to this message.
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. On Mon, Mar 25, 2013 at 1:57 PM, <usrbincc@yahoo.com> wrote: >> 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/SourceM... > File src/outputgeneration/SourceMapIntegration.js-template.js (right): > > https://codereview.appspot.com/7789051/diff/7001/src/outputgeneration/SourceM... > 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/SourceM... > 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
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
