Bump Traceur to 0.0.82 #80
Conversation
* Generalize es6ify() argument from filePattern => opts, with
fallback to previous behavior if a regex is passed. Now filePattern
should be specified as {filePattern: ...}.
* Support opts.sourceRoot param to pass to traceur. It now appears
to be neccessary to specifify this to traceur to get correct paths
in source map.
|
I forgot to mention, it may be desirable to default |
|
Nice work @jmm! WRT plugin, would totally love a PR that makes this work as plugin while keeping it working as transform as well. Something similar was done(among other fixes) to proxyquireify a while ago. |
|
Thanks @thlorenz!
It shouldn't. As long as
I might be able to work on that. Thanks for the link, I'll take a look. I've done some work along those lines (turning a transform into a plugin) on other projects. How do you feel about reading browserify's |
|
I pushed a couple more commits that are oriented toward getting the correct file name in the source map (e.g. not exposing absolute local paths). They are hasty and rough and definitely need more review. There'll probably need to be separate tests for with and without |
| function es6ify(filePattern) { | ||
| filePattern = filePattern || /\.js$/; | ||
| function es6ify(opts) { | ||
| opts || (opts = {}); |
There was a problem hiding this comment.
Please use a proper if or ?:. Also best to follow ES6 and do if (opts === undefined) { opts = {}; } instead of using any falsy value.
There was a problem hiding this comment.
How about just opts = opts || {}?
Unless I"m missing something could opts ever be anything but an object?
There was a problem hiding this comment.
Not ES6ey enough, only undefined should trigger defaults :)
There was a problem hiding this comment.
OK, let's agree to disagree on this one then. I feel the verbosity isn't worth the es6iness.
However doesn't matter enough to me to fight you on that :)
In either case we should make this consistent across the repo, no matter which way we pick.
There was a problem hiding this comment.
Also best to follow ES6 and do if (opts === undefined) { opts = {}; } instead of using any falsy value.
Not ES6ey enough, only undefined should trigger defaults :)
I don't see what it accomplishes -- the new test will still allow a bunch of nonsensical values, and convert es6ify(null) (which admittedly doesn't make sense) to a TypeError -- but it's up to you guys. Also, if making this codebase ES6ey is an imperative, why not actually author it in ES6 and transpile it?
Please use a proper
ifor?:
You're referring to readability preferences not functionality, right? A ?: in this situation seems like the worst option to me.
There was a problem hiding this comment.
Yes, TypeError sounds great, and yes, readability.
|
Left some comments. Overall looks good, although I am curious about taking this opportunity to convert to a plugin. (I don't see a need for it to be a transform if it is a plugin, personally.) In that case we should also automatically include the runtime, as it's basically nonsensical to use Traceur without it. It sounds like the source map filename stuff is a mess so I'll trust you to be doing the right thing there. Wish Traceur just did the right thing automatically. Maybe they should try copying 6to5. |
|
@domenic Thanks for the feedback. I'll fix anything that needs fixing.
I think you're saying you're in favor and you'd prefer it to be only a plugin from now on (breaking backward compatibility by removing the option to use it as a transform)? The only reason I'd give for making it a plugin currently is to access browserify's
I'm currently using it on a project purely to transpile ES6 module syntax, for which I don't need or want the runtime.
Well I think the addition of the |
|
For a test? The latter seems better. |
Yes, both snippets are in the test file I linked to. |
|
Here's the rundown on how the paths work out:
In all cases the resulting source map output by browserify has |
|
Created #81 for further discussion of conversion to a plugin. |
|
Regarding docme and jsdoc, that's all @thlorenz, I am not a jsdoc fan and just try to copy what I see around me :P. The |
Ok, thanks. Hopefully @thlorenz can point me in the right direction.
Like I said, until the last minute I had it as |
|
sourceMapBase or sourceMapBaseDir? |
|
I sort of feel like prefixing any source map-related opts with |
|
@jmm added you as collab, so once we agree on everything you could squash and forward merge into master. |
|
I'm going to leave some notes here for future reference. The current pull request brings traceur up to 0.0.82 with proper
So if that all works out there probably shouldn't be separate tests anymore for with / without the |
Ok. Sorry @guzart, didn't realize you'd already done so much work in this direction. I skimmed #25 (but haven't looked at any of the commits) and it sounds similar to what I've been working on in #81. I think I'll clean up what I have so far a little but and push it so everyone can take a look and if @guzart wants to take the reins again can cherry pick anything good from there. One thought I do have right off the bat is that I think |
Ok, thanks. I kind of think the history is useful, but it's up to you. If you want me to squash, all into one commit?
Ok, I'll see what I can do.
Thanks. |
|
@jmm squash as you see fit. |
Since runtimes will be on by default it should be false by default, so e.g. |
|
I think a double negative is more confusing (do not do it? yes) than a straightforward: do it? -- yes or no -- that defaults to yes. But, it's up to you. |
|
Yeah I feel pretty strongly that boolean options should have a default value of false. If you can think of a better phrasing that's cool (e.g. maybe "omit" is better than "no"). |
|
I think no matter what you name it it's more confusing than the straightforward version. I'll let you know if I have a suggestion for a default: |
|
FYI, traceur 0.0.83 is out now and partially fixes the problems with |
|
So there's been a lot of back-and-forth. At some point we should merge and do a release :). What are the unresolved issues remaining? (Apologizes if you're waiting on me or @thlorenz to respond to something that got lost in the shuffle...) |
|
unfortunately :( there isn't much I can bring in to the conversation, main points have already been discussed:
|
|
Ok, I think these are the unresolved issues: For this pull request:
Here's a summary of issues that have been discussed in or are related to this thread that relate to conversion to a plugin and should be hashed out in #25 or #81:
|
Can you explain why My reasoning for something more explicit like "sourceMapBaseDir" is that (a) I don't mind if people type extra characters; (b) it's very clear what it does; (c) it explicitly lets you know this is only related to source maps and not to all "sources" or to es6ify in general. |
It means the path.relative(basedir, file)Where
A) In my opinion, all else being equal (including descriptiveness), shorter is preferable. B) I'm not so sure it is. It's not used in any other context (like the source map spec or mozilla/source-map), is it?, and there are a number of paths / URLs related to source maps: C) I guess. There's only a handful of parameters for es6ify and the user isn't going to know what they are or how to use them in the first place without looking at the documentation. |
|
We are experience these issues, is this PR going in or are you waiting for further traceur updates? including traceur 0.0.83 which includes other related changes? @jmm is there any workaround for the time being to get a working (in chrome) source map? |
I personally am waiting for some combination of traceur maturing its API with respect to source maps... and continuation of discussion here about the unresolved issues in this thread.
Here's the workaround I'm using to deal with google/traceur-compiler#1748 / chromium # 459682 (which was bogusly merged with another issue): var rs = require('readable-stream');
var stream = new rs.Transform;
stream.data = '';
stream._transform = function (chunk, enc, cb) {
this.data += chunk;
cb();
};
stream._flush = function (cb) {
this.data = this.data.replace(/\/\/# sourceURL=\S+/mg, '');
this.push(this.data);
cb();
};
return b
.transform(es6ify)
.bundle()
.pipe(stream)And here's the workaround I'm using to correct the paths in the source map: gulp.task('build:fix-source-maps', ['build:bundle'], function (done) {
if (! use_source_map) return;
var map_file = './dist/rsrc/app.js.map';
fs.readFile(map_file, function (err, src) {
if (err) throw err;
var map = JSON.parse(src);
map.sources = map.sources.map(function (source) {
var
base_dir = [__dirname, 'src'],
ret = source;
[
base_dir,
base_dir.slice(0, -1),
base_dir.slice(-1)
].every(function (base_dir) {
base_dir = path.join.apply(path, base_dir);
if (ret.indexOf(base_dir + '/') === 0) {
ret = path.relative(base_dir, ret);
return false;
}
return true;
});
return ret;
});
fs.writeFile(map_file, JSON.stringify(map, null, 2), done);
});
}); |
This is a work in progress, but I wanted to get the discussion going. It appears to now be necessary to pass a
sourceRootparam to traceur to get correct paths in source maps. So I think it's time to make the options toes6ify.configure()more general or turn this into a plugin. In this pull request I've implemented the former approach, turning thefilePatternparam intoopts(which can currently takefilePatternandsourceRootproperties). It falls back to the previous behavior if a regex is passed. (I duck typed it based onignoreCase-- cool?) I rearranged some of the internal options passing based on that change and the addition ofsourceRootto the mix.