Make 'traceur' cleanup process.argv and setup the node.js module so that relative require works.

Make 'traceur' cleanup process.argv and setup the node.js module so that relative require works.


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

[arv, 2013-03-27 14:39:52]

I'm not sure why we need to do the process.argv manipulation in command.js?

Also, I need you to fill out AUTHORS and sign the CLA. See th e wiki for link to
the CLA: https://code.google.com/p/traceur-compiler/wiki/Contributions

https://codereview.appspot.com/7552046/diff/1/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/1/src/node/command.js#newcode94
src/node/command.js:94: process.argv = process.argv.slice(0,
1).concat(includes);
The idea was that interpreter.js would do this.

https://codereview.appspot.com/7552046/diff/1/src/node/interpreter.js
File src/node/interpreter.js (right):

https://codereview.appspot.com/7552046/diff/1/src/node/interpreter.js#newcode27
src/node/interpreter.js:27: require.main.filename = filename;
makes sense

[eddyb, 2013-03-27 14:55:55]

https://codereview.appspot.com/7552046/diff/1/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/1/src/node/command.js#newcode94
src/node/command.js:94: process.argv = process.argv.slice(0,
1).concat(includes);
On 2013/03/27 14:39:52, arv-chromium wrote:
> The idea was that interpreter.js would do this.
interpreter.js doesn't know which arguments are flags and where actual arguments
start for the script running under traceur. includes is the array containing all
the arguments following traceur options.

[arv, 2013-03-27 16:25:37]

Sorry, I should have looked closer the first time. I added some comments on the
bug.

https://codereview.appspot.com/7552046/diff/6001/src/node/interpreter.js
File src/node/interpreter.js (left):

https://codereview.appspot.com/7552046/diff/6001/src/node/interpreter.js#oldc...
src/node/interpreter.js:28: argv[0] = 'traceur';
The idea was that argv would work like without traceur where it replaces 'node'
with 'traceur'

[usrbincc, 2013-03-27 16:48:23]

Note: I'm mostly playing the role of opinionated observer here. arv is
the main reviewer here.

Thanks for getting to this bug. It's one that I've had my eye on for
awhile, but haven't been able to get to.

The first patch (maybe second or third, actually) is the hardest, so
please stick with it.

A full fix for the arg-handling bugs would address all the errors in my
later comment. I think the arg fix works in more cases than our current
code, but it still has problems. If this code is accepted, there needs
to be a follow-up eventually.

The 'require.main.filename' fix seems fine from the brief testing I did.
I'm not too familiar with node's magic internals.

https://codereview.appspot.com/7552046/diff/1/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/1/src/node/command.js#newcode94
src/node/command.js:94: process.argv = process.argv.slice(0,
1).concat(includes);
> interpreter.js doesn't know which arguments are flags and where actual
> arguments start for the script running under traceur. includes is the
> array containing all the arguments following traceur options.

There are actually a host of bugs in the arg handling code. Most of them
have to do with the Commander.js library we use for arg handling.

#--cut--
## try this code with the unpatched version. You can see what's going on
## more clearly, since argv is only slightly modified.

cat > argv.js <<"END"
console.log(process.argv) // no semicolon on purpose
END

## bug. should interpret
./traceur argv.js --out argv.out.js

## bug. should not process args following filename.
./traceur argv.js --strict-semicolons

## let's see what 'includes' contains.
sed -e '/var includes =/a\
console.log("includes: %s", includes); // REMOVEME' \
  -i src/node/command.js

## 'includes' is actually just the leftover pieces that aren't attached
## to an option or something that looks like an option.
./traceur argv.js --some-random-option file1.js file2.js

## remove our debug code.
sed -e '/REMOVEME/d' -i src/node/command.js
#--cut--

  See the discussion here for the previous discussion about these bugs:

  Command line traceur will now invoke the interpreter if no out param is passed
  https://codereview.appspot.com/7810043

The options (no pun intended) are to patch the upstream library to allow
the behavior we want, or to write our own code. I'm personally leaning
towards trying to patch upstream, since my first github pull request
went so well.

  Update. After looking through the Commander.js code, it seems
  needlessly complex with a lot of things we don't use. I'm still
  leaning towards it, but it's more out of a sense of contributing to a
  library used by many, rather than getting from point A to point B in
  the shortest time.

https://codereview.appspot.com/7552046/diff/1/src/node/command.js#newcode94
src/node/command.js:94: process.argv = process.argv.slice(0,
1).concat(includes);
On 2013/03/27 14:39:52, arv-chromium wrote:
> The idea was that interpreter.js would do this.

You know, I wonder if something in the spirit of 'execv'

  int execv(const char *path, char *const argv[]);

might not divide the responsibility better. I mean, that's essentially
what 'interpret' does. At least in effect, even if not in the details.

  // So these would be conceptually equivalent:

  // C
  char **newArgv = munge(argv);
  execv(filename, newArgv);

  // JS
  var newArgv = munge(process.argv);
  interpret('traceur', includes[0], newArgv);

  // In this example, 'interpret' would just do a simple
  function interpret(interpreter, filename, newArgv) {
    ...
    var argv = [interpreter, filename];
    argv.push.apply(argv, newArgv);
    process.argv = argv;
    ...
  }

[usrbincc, 2013-03-28 15:48:24]

By the way, reviewers don't get notifications unless you do a
"Publish+Mail Comments" -- even if it's just an empty one.

Most of the final issues from now on will probably be about coding
style and convention. Sometimes it seems silly, but there's a certain
discipline in coding in a style different from your usual one (you may
see my preferred style peeking out occasionally).

arv has the final say on this one, but I'll try to cover some of the
formatting and coding style issues I know he'll probably bring up
(because I've had to fix them in my own code reviews).

----

The code I suggested below is untested, but it should be enough to get
the general idea.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode70
src/node/command.js:70: // without the file name and anything past that.
Traceur coding style uses JSDoc for most functions (I notice that we're not
always consistent about it, but still). Since this is your first patch,
here's a template:

/**
 * HACK: Process arguments so that in interpret mode, commander.js only sees the
 * flags, without the file name and anything past that. If an invalid flag is
 * encountered, only the arguments up to the invalid flag are passed on. This is
 * necessary in order to reliably trigger commander.js's error reporting.
 * @param {Array.<string>} argv
 * @return {Array.<string>}
 */

I did a quick update to the comment to match the current expected
functionality. Reword or change however you want.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
src/node/command.js:77: if (arg == '--' || arg == '--out')
traceur coding style uses '===' and '!=='.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode85
src/node/command.js:85: arg = argv[i+1];
argv[i + 1];
spaces around operators.

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode93
src/node/command.js:93: argv.splice(i, 0, '--');
I had a closer look through commander.js and I found out why invalid
flags weren't being flagged (so to speak). Turns out it's supposed to be
a feature.

If there are any extra pieces that aren't flag-like, then it doesn't
consider the invalid flags as errors. So the only way to mark them as
errors is to filter out the extra pieces that aren't flag-like. And
even then, you're only going to get an error for the first invalid flag.

  'gcc' is much nicer, and flags all invalid flags. By default, it
  doesn't assume invalid flag take args, either.

In that case, you might as well not bother going to the trouble of doing
the filter at all, and just return the first invalid arg.

  // If it's an invalid flag, we want it to show up as an error. Due to
  // a quirk in commander.js, we only get an error message for the first
  // invalid flag, so just return that one and don't bother finding the
  // rest.
  if (arg[0] === '-')
    argv.splice(i + 1);
  else
    argv.splice(i, 0, '--');

To be honest, this library has made some interesting design decisions.

'processArguments' should probably be marked 'HACK' to make these things
easier to find for later cleanup.

----

I'm still trying to figure out what would be the cleanest way to fix
these bugs upstream. This is what I'm currently considering:

- An option to make unknown flags that occur before the first extra
  regular arg into errors, even though I really think this should always
  be the case. So maybe I won't even make it an option, and see if that
  flies.
- The ability to set a callback for when the first regular arg is
  encountered.
- Have the ability for option callbacks to modify later arg parsing.
  Specifically, the ability to force the rest of the args to be pushed
  verbatim into flags.args

----

Just realized. There is a bug in the code. Or rather a bug in the workaround
for commander.js's bugs.

  ## bug, should error out on '--invalid-arg'.
  ./traceur --out file.out.js file.js --invalid-arg
  ./traceur --out file.out.js --invalid-arg file1.js file2.js

Honestly, this is getting hackier and hackier all the time, but we've gone
this far already, so it's not too much more code:

  if (arg === '--out') {
    i++;
    compile = true;
  }

  ...

  // HACK: Argh!!!!! HACKHACKHACK
  if (arg[0] === '-') {
    argv.splice(i + 1);
    break;
  } else if (!compile) {
    argv.splice(i, 0, '--');
    break;
  }

[arv, 2013-03-28 16:19:38]

To be honest I don't understand all these issues. I'm fine checking this in as
is (with Ben's comments resolved).

Another thing that would make me feel better about this is tests. Currently we
have no test for these things. I'll file bugs.

If CommanderJS is not fitting our needs I'm fine changing to something that
works better or for us to create something minimal that just covers our needs.

[arv, 2013-03-29 15:20:56]

LGTM

I'll commit this to Google Code and GitHub. Future patches should go to GitHub.

It would also be good to get rid of CommanderJS because it is now clear it does
not suite our needs well.

[arv, 2013-03-29 15:25:23]

Committed as 8a32965d92cf75d98490b1a8513402dfd55f4db1

[arv, 2013-03-29 15:26:18]

One more thing. Only the owner of the code review issue can close it. Please
close this issue (circle with x in upper left corner)

[eddyb, 2013-03-29 15:33:03]

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
File src/node/command.js (right):

https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
src/node/command.js:77: if (arg == '--' || arg == '--out')
On 2013/03/28 15:48:24, usrbincc wrote:
> traceur coding style uses '===' and '!=='.

Random information: I've done some tests on jsperf, and in v8, === and !== are
slower for comparing strings and numbers (when both == and === would give
correct results in all situations), but they're faster when used with typeof.

[arv, 2013-03-29 15:37:06]

On 2013/03/29 15:33:03, eddyb wrote:
> https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
> File src/node/command.js (right):
> 
>
https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
> src/node/command.js:77: if (arg == '--' || arg == '--out')
> On 2013/03/28 15:48:24, usrbincc wrote:
> > traceur coding style uses '===' and '!=='.
> 
> Random information: I've done some tests on jsperf, and in v8, === and !== are
> slower for comparing strings and numbers (when both == and === would give
> correct results in all situations), but they're faster when used with typeof.

This is mostly about style. Google JS style is to prefer == over === but most of
the JS community prefers === over ==.

=== should not be slower. Can you point me at the jsperf link and I'll file V8
bugs.

[usrbincc, 2013-03-29 16:43:26]

Congratulations on your first patch. Welcome to AUTHORS.

Sorry, just a little enthused because you're the first new person to
send in a patch since ... me.

The pace should pick up with the move to github, so I'm sure the novelty
will wear off.

[eddyb, 2013-03-29 17:01:57]

On Fri, 29 Mar 2013, 17:37:06 EET, arv@chromium.org wrote:

> On 2013/03/29 15:33:03, eddyb wrote:
> > https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
> > File src/node/command.js (right):
> 
> 
>
https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
> > src/node/command.js:77: if (arg == '--' || arg == '--out')
> > On 2013/03/28 15:48:24, usrbincc wrote:
> > > traceur coding style uses '===' and '!=='.
> 
> > Random information: I've done some tests on jsperf, and in v8, === and
> !== are
> > slower for comparing strings and numbers (when both == and === would
> give
> > correct results in all situations), but they're faster when used with
> typeof.
> 
> This is mostly about style. Google JS style is to prefer == over === but
> most of the JS community prefers === over ==.
> 
> === should not be slower. Can you point me at the jsperf link and I'll
> file V8 bugs.
> 
> https://codereview.appspot.com/7552046/

Well, I remembered it from a couple of months ago, I guess Chrome 26 looks
fixed:
http://jsperf.com/comparison-of-comparisons

[eddyb, 2013-03-29 17:22:34]

On Fri, 29 Mar 2013, 18:43:26 EET, usrbincc@yahoo.com wrote:

> Congratulations on your first patch. Welcome to AUTHORS.
> 
> Sorry, just a little enthused because you're the first new person to
> send in a patch since ... me.
> 
> The pace should pick up with the move to github, so I'm sure the novelty
> will wear off.
> 
> 
> https://codereview.appspot.com/7552046/

Thanks, I've been watching the project for more than a year now :).
If things are keeping up for me, I'll try to work on my ridiculous projects and
contribute to traceur.

[arv, 2013-03-29 17:29:37]

Filed https://code.google.com/p/v8/issues/detail?id=2601

On Fri, Mar 29, 2013 at 1:01 PM,  <edy.burt@gmail.com> wrote:
> On Fri, 29 Mar 2013, 17:37:06 EET, arv@chromium.org wrote:
>
>> On 2013/03/29 15:33:03, eddyb wrote:
>> > https://codereview.appspot.com/7552046/diff/11001/src/node/command.js
>> > File src/node/command.js (right):
>>
>>
>>
>>
https://codereview.appspot.com/7552046/diff/11001/src/node/command.js#newcode77
>> > src/node/command.js:77: if (arg == '--' || arg == '--out')
>> > On 2013/03/28 15:48:24, usrbincc wrote:
>> > > traceur coding style uses '===' and '!=='.
>>
>> > Random information: I've done some tests on jsperf, and in v8, === and
>> !== are
>> > slower for comparing strings and numbers (when both == and === would
>> give
>> > correct results in all situations), but they're faster when used with
>> typeof.
>>
>> This is mostly about style. Google JS style is to prefer == over === but
>> most of the JS community prefers === over ==.
>>
>> === should not be slower. Can you point me at the jsperf link and I'll
>> file V8 bugs.
>>
>> https://codereview.appspot.com/7552046/
>
> Well, I remembered it from a couple of months ago, I guess Chrome 26 looks
> fixed:
> http://jsperf.com/comparison-of-comparisons



-- 
erik

[arv, 2013-03-29 17:31:39]

Congratulations from me too.

There are a lot of kinks to work out to make Traceur go from a
research project to something useful. More people trying it out will
definitively expose these issues.


On Fri, Mar 29, 2013 at 1:22 PM,  <edy.burt@gmail.com> wrote:
> On Fri, 29 Mar 2013, 18:43:26 EET, usrbincc@yahoo.com wrote:
>
>> Congratulations on your first patch. Welcome to AUTHORS.
>>
>> Sorry, just a little enthused because you're the first new person to
>> send in a patch since ... me.
>>
>> The pace should pick up with the move to github, so I'm sure the novelty
>> will wear off.
>>
>>
>> https://codereview.appspot.com/7552046/
>
> Thanks, I've been watching the project for more than a year now :).
> If things are keeping up for me, I'll try to work on my ridiculous projects
> and contribute to traceur.



-- 
erik