Mostly looks okay. The only issues are with the arg handling and needing
the 'require' to make compiler.js work standalone.
https://codereview.appspot.com/7810043/diff/4001/src/node/command.js
File src/node/command.js (right):
https://codereview.appspot.com/7810043/diff/4001/src/node/command.js#newcode38
src/node/command.js:38: flags.option('--traceur-options', 'xxxxx');
flags.option('--traceur-options <OPTIONS>', 'specify feature options');
I'm wondering about this, though. You can't pass '--traceur-options' in
the hypothetical '#!/usr/bin/env traceur' case.
It's possible for '#!/usr/bin/traceur --traceur-options=opt1,opt2=false'
but it's unlikely traceur can reliably get to a standard location on all
systems.
Hypothetical at the moment, since this code doesn't handle shebang yet.
----
On the other hand, I'm not sure this will ever see common use, since in
the normal case, you can just specify args directly.
./traceur --strict-semicolons --block-binding file.js
Maybe just remove '--traceur-options' and implement 'Options:' in a
follow-up?
https://codereview.appspot.com/7810043/diff/4001/src/node/command.js#newcode40
src/node/command.js:40: console.log(arguments);
Noting debug code just in case.
https://codereview.appspot.com/7810043/diff/4001/src/node/command.js#newcode75
src/node/command.js:75: flags.parse(process.argv);
A little gotcha with passing args on to the script:
# compiles instead of interpreting
./traceur interp-test.js --out out.js
# this works
./traceur -- interp-test.js --out out.js
'--' works, but most interpreters I know handle this without
needing help. Probably just ignore for now, since this cannot be
separated from the fact that we use Commander.js .
https://codereview.appspot.com/7810043/diff/4001/src/node/compiler.js
File src/node/compiler.js (left):
https://codereview.appspot.com/7810043/diff/4001/src/node/compiler.js#oldcode30
src/node/compiler.js:30: require('./traceur.js');
Will compiler.js ever be used separately from command.js? If so, this
still needs to be in here. According to node docs, this should only get
executed once, even if you 'require' several times, so it shouldn't hurt
to leave it in.
https://codereview.appspot.com/7810043/diff/4001/src/node/compiler.js
File src/node/compiler.js (right):
https://codereview.appspot.com/7810043/diff/4001/src/node/compiler.js#newcode91
src/node/compiler.js:91: inlineAndCompile(includes.slice(current,
current + 1), {}, reporter,
Not sure if compiler.js will ever need to pass any flags, but just
something to note.
https://codereview.appspot.com/7810043/diff/4001/src/node/interpreter.js
File src/node/interpreter.js (left):
https://codereview.appspot.com/7810043/diff/4001/src/node/interpreter.js#oldcode31
src/node/interpreter.js:31: var argv = process.argv.slice(1);
Note that process.argv still contains any options passed to the
interpreter. Easiest thing would be to just use 'flags.args' from
commander.js, the only caveat being that you need to prepend 'traceur'.
Up to you on that, but I have graciously provided my opinion in the
code comment following this one.
https://codereview.appspot.com/7810043/diff/4001/src/node/interpreter.js#oldcode32
src/node/interpreter.js:32: argv[0] = 'traceur';
I know this mirrors node convention:
['node', 'file.js', 'arg1', 'arg2']
['traceur', 'file.js', 'arg1', 'arg2']
but I still prefer the bash and python convention:
['file.js', 'arg1', 'arg2']
though ruby and perl inexplicably do:
['arg1', 'arg2']
Couldn't resist a mini-rant. But this is node convention, so there is
some logic to it.
https://codereview.appspot.com/7810043/