search

ericmckean / traceur-compiler

Automatically exported from code.google.com/p/traceur-compiler
Apache License 2.0
0 stars 0 forks source link

Node command.js/interpreter.js leave traceur flags in process.argv and break relative require. #226

Open GoogleCodeExporter opened 9 years ago

GoogleCodeExporter commented 9 years ago 
For the first problem, this is a good fix:
  process.argv = process.argv.slice(0, 2).concat(includes);
(just before interpret(includes[0]), in command.js)
There's an incorrect fix in interpreter.js, that isn't aware of traceur 
arguments (flags), and should be removed.

For the second one, the easiest fix seems to be using module._compile instead 
of require.main._compile, in interpreter.js. Or setting require.main.filename 
instead of module.filename.
The module _compile is called on needs to have the filename set, so require 
within the module works with relative paths.

Original issue reported on code.google.com by edy.b...@gmail.com on 27 Mar 2013 at 1:25

GoogleCodeExporter commented 9 years ago 
Do you mind providing a patch?

https://code.google.com/p/traceur-compiler/wiki/Contributions

Original comment by arv@chromium.org on 27 Mar 2013 at 1:38

GoogleCodeExporter commented 9 years ago 
Done (it's my first time, I hope it went well)

https://codereview.appspot.com/7552046

Original comment by edy.b...@gmail.com on 27 Mar 2013 at 2:04

GoogleCodeExporter commented 9 years ago 
Let me try to clarify some of the requirements. I should really add a test that 
enforces these.

$ ./traceur test.js --abc def.js

In test.js

var assert = require('assert');
assert.equal(process.argv[0], 'traceur');
assert.equal(process.argv[1], 'test.js');
assert.equal(process.argv[2], '--abc');
assert.equal(process.argv[3], 'def.js');
assert.equal(__filename, 'test.js');

With your patch this fails

Original comment by arv@chromium.org on 27 Mar 2013 at 4:25

GoogleCodeExporter commented 9 years ago 
test.js:
console.log(process.argv, __filename);

$ ./traceur test.js --abc def.js
[ 'node', 'test.js' ] 'test.js'

__filename is right with my patch and process.argv[0] is easy to fix.

The bigger problem here is the missing --abc def.js. I'm afraid commander won't 
let us stop parsing arguments after the first non-flag argument, but I'll have 
a look into it.

Original comment by edy.b...@gmail.com on 27 Mar 2013 at 4:37

GoogleCodeExporter commented 9 years ago 
The way to look at this is that the flags are not traceur flags, but flags to 
test.js. We've discussed adding a --traceur-flags="--experimental --debug" to 
allow passing flags to traceur (similarly to how node allows you to pass flags 
to v8)

Original comment by arv@chromium.org on 27 Mar 2013 at 4:41

GoogleCodeExporter commented 9 years ago 
Node lets you pass v8 flags directly, I think Chrome/Chromium require wrapping 
in another flag.

After reading Commander.js internals, I came up with a hack-ish way of 
splitting the arguments into traceur flags and script names + script flags.
https://gist.github.com/eddyb/9a4fd8f668828dbfeda5#file-command-js-L68

Do you consider this to be a proper solution? I won't pollute the patch if it 
isn't.

Original comment by edy.b...@gmail.com on 27 Mar 2013 at 5:10

GoogleCodeExporter commented 9 years ago 
I'm going to try to keep the conversation here and on the issue page
instead of adding a third location (github). Unless it becomes
inconvenient.

The code you wrote seems fine for interpreting-only, but seems like it'd
fall down with something like:

  ## note: untested. And this might be considered a pathological case.
  ## '--strict-semicolons' taken to be a filename.
  ./traceur --out argv.out.js argv.js --strict-semicolons

That aside, if everything worked correctly, I'd be inclined to accept
that as a temporary fix, and eventually have the goal of either getting
a proper patch into Commander.js, or doing our own arg-handling (My
thoughts on these options are on the issue page.

My main problem with hacking it is that it's the right thing to do if
you're working around a broken API -- but we don't have to live with
broken APIs. We can submit patches, or we can use our own code.

You could modify the code to fix the bug I mentioned earlier, but then
you're just recreating more and more of the functionality of
Commander.js, at which point we might as well use that instead.

I'm still considering that possibility...

Original comment by usrbi...@yahoo.com on 27 Mar 2013 at 5:38

GoogleCodeExporter commented 9 years ago 
I'm only using gists to share my code, rather than paste it here.
I thought about the --out problem before, but I didn't realize the solution was 
easy (although it's a hack in the hack): 
https://gist.github.com/eddyb/9a4fd8f668828dbfeda5/revisions

I guess I'll wait now for your decision.

Original comment by edy.b...@gmail.com on 27 Mar 2013 at 6:23

GoogleCodeExporter commented 9 years ago 
I actually kind of like the github interface, especially the fact that
anyone can fork and put changes up for comment. It's good for quick
feedback.

There was some talk before about moving traceur to github, but it looks
like we're sticking with codereview for now. It's a little clunky, but
it works well enough for most things. Do you mind updating codereview
with your latest?

----

https://gist.github.com/usrbincc/346fb18c8b963dc85046

I made some changes, mostly to make it easier to put it into its own
function. I added some code to pass directly to Commander.js if the
first non-option is an arg-like.

This still doesn't handle

  ./traceur --invalid-arg --out argv.out.js argv.js

but that's because Commander.js has something borked somewhere. Or maybe
I'm not setting a flag somewhere. No time to check more closely.

----

I'll be out for awhile, so won't be able to reply.

Code review is a lot slower than github. You get used to it eventually.
One strategy that mitigates it is parallelism.

Original comment by usrbi...@yahoo.com on 27 Mar 2013 at 7:24

GoogleCodeExporter commented 9 years ago 
I was going to commit the (hopefully) last change in the patchset, but I 
noticed commander.js has a very weird "unknown option" policy.

With flags.parse(process.argv), this is the result:
$ echo > test.js
$ ./traceur test.js -foo
$ ./traceur test.js --foo
$ ./traceur test.js --foo bar
$ ./traceur test.js --foo
$ ./traceur --foo test.js

  error: unknown option `--foo'

$ ./traceur --foo bar test.js
$ ./traceur --foo bar --baz test.js

  error: unknown option `--foo'

$ ./traceur --foo bar --baz boo test.js

I guess this is designed to be way more flexible than traceur needs, so I'll 
adapt the already hacky function to perform error reporting, too.

Original comment by edy.b...@gmail.com on 29 Mar 2013 at 3:01