Node API: add options parameter to require.makeDefault() #1166

UltCombo · 2014-07-09T04:46:29Z

Closes #1162.

I've signed the CLA using my email ( ultcombo [\at] gmail {!dot!} com )

I'm unsure how to properly add a test for require.makeDefault(), as there is no way to "resetDefault()". Should we create a method for that as well?

UltCombo · 2014-07-09T05:29:32Z

Added tests.

I've enabled blockBinding in the tests. However, seeing as it will likely be enabled by default in the future, maybe I should pass an already enabled by default option as false and check if it throws errors instead?

@johnjbarton would you be able to review this when you have time? Thanks.

johnjbarton · 2014-07-09T14:18:48Z

test/unit/node/require.js

+      {blockBinding: true});
+    // TODO(arv): The path below is sucky...
+    var x = require(path.join(__dirname, './resources/let-x.js')).x;
+    assert.equal(x, 'x');


At this point we have mutated nodejs's configuration via makeDefault. We should undo that mutation with another call to makeDefault and test that let now fails.

You could add this same let-fails test before your call to makeDefault to verify that blockBinding is still an option that is false by default.

johnjbarton · 2014-07-09T14:26:56Z

Thanks for this! Two review comments.

UltCombo · 2014-07-09T18:32:51Z

@johnjbarton oh sorry for spamming the commits, by the way. I'm using Travis to run the tests as I don't have Make at the moment. Guess I'll boot up my Linux VM for the next contributions.

johnjbarton · 2014-07-09T18:58:52Z

src/node/require.js

  if (!results.js)
    console.error(results.errors);

  return results.js;
 }

-function traceurRequire(filename) {
-  var source = compile(filename);
+function traceurRequire(filename, options) {


I'm just unsure about this change. If a user sets options here and the code contains require() without options then what happens?

traceur.require() only uses the options argument to compile the requested file, it will never affect other traceur.require nor the default require calls as far as I can see. If no options are passed, it will be treated the same as calling .compile() without options -- default parameters will kick in when we pass an undefined options argument into .compile().

I can remove that part if you'd like, though it could be useful in some use cases.

@johnjbarton I'm assuming you mean a function call such as traceur.require('foo.js').

In that case, the options argument will be undefined. This proposed change will pass through the options argument (object or undefined) to Compiler#compile, where the options argument will default to an empty object if the function call does not provide a value or provides the undefined value.

Is there any doubts about this process?

Oh woops, checking it again, traceur.require actually forwards the options argument to Compiler.commonJSOptions, which will also default to an empty object too anyway.

It is the first case you outlined that concerns me:

traceur.require() only uses the options argument to compile the requested file, it will never affect other traceur.require nor the default require calls as far as I can see

Users may be confused if they set an option through this API. Let's remove it until we see use cases for sure.

Makes sense, I'll do it right away.

UltCombo · 2014-07-09T18:59:32Z

@johnjbarton alright, pushed your requested changes. By the way, should we merge the "traceurRequire options" test into the "traceurRequire.makeDefault options" test as well? Or should we move the fixturePath and experimentalOption to a scope above in order to DRY up both tests?

arv · 2014-07-09T19:51:28Z

@UltCombo You can get make for windows. I used one of these (I don't remember which one)

https://code.google.com/p/msysgit/issues/detail?id=278

UltCombo · 2014-07-09T19:56:30Z

Thanks @arv! Guess I was just a bit too lazy to look it up for the first PR test, but it will undoubtedly speed up my workflow here now.

UltCombo · 2014-07-09T20:01:14Z

Damn typos, guess I'll go home and take a nap soon -- can't really be very productive with sleep deprivation.

@johnjbarton If you're unsure about the options argument in the traceur.require() API, we can remove it. I don't quite need it personally, but some folks out there would most likely find some usage for it, and adding that parameter does not affect any existing code as far as I can see. Up to you though.

UltCombo · 2014-07-09T21:25:59Z

Alright, cleaned up the code and updated the PR summary.

By the way, in regards to the contributing guide:

The pull request message body will be used as the commit message.

Does "message body" refer to the PR summary (first line) or does it also includes the message body (which Github displays as "first comment")? If it is the latter, I'll update the PR body/first comment as well.

arv · 2014-07-09T22:38:41Z

The first comment.

Alternatively you can squash all commits into one with the desired commit message.

arv · 2014-07-09T22:40:42Z

LGTM

Let me know when you've done one of the above and I can merge this for you (I see that you've signed the CLA already).

UltCombo · 2014-07-10T10:19:55Z

@arv done, I've just squashed all the commits. =] I've also amended the author signature in order to match the email which I've signed the CLA with.

arv · 2014-07-10T15:35:12Z

I'm seeing extra output in the result:

https://travis-ci.org/google/traceur-compiler/builds/29587370#L470

Can this be removed?

UltCombo · 2014-07-10T17:34:11Z

I'll be a bit busy in the next hours, I'm not sure why that line is appearing. Wouldn't it be related to the unit testing framework?

arv · 2014-07-10T18:21:38Z

It looks like the code gets executed and the error message is from that. If the options got updated correctly then the Traceur parser would raise the error.

UltCombo · 2014-07-10T19:07:29Z

@arv sorry, not sure if I understood correctly. Do you mean that the Traceur parser logs the error whenever it fails? If so, we could think of something to replace the try/catch tests.

arv · 2014-07-10T19:13:08Z

It looks like the code parses fine by Traceur. The error comes when Node.js tries to execute the code. I thought the goal of this test was that we can reset the traceur options back, which should cause an error in Traceur's parser which gets reported through the ErrorReporter and the code never gets executed.

UltCombo · 2014-07-10T20:33:19Z

@arv yes, I expected that to be the case too. I'm a bit busy atm, I'll probably be able to take a look later tonight. =]

arv · 2014-07-10T20:39:46Z

Thanks

johnjbarton · 2014-07-10T20:58:56Z

The compile function writes to the console:
https://github.com/UltCombo/traceur-compiler/blob/node-require-makedefault-options/src/node/require.js#L32

Unfortunately if you throw instead of write the console I don't think you will see the compile errors.

UltCombo · 2014-07-10T21:10:58Z

Oh I see, thanks for the quick analysis @johnjbarton. Is there something we should change there? Doesn't ErrorReporter display those errors? (sorry, I haven't checked it yet)

johnjbarton · 2014-07-10T21:19:11Z

Please change compile() to throw rather than call console.error. It's ok if you just throw result.errors[0].

UltCombo · 2014-07-10T21:32:46Z

@johnjbarton ok, done and squashed. =]

UltCombo · 2014-07-10T21:56:26Z

@arv are you okay with this change? If you'd rather display all errors, maybe we could throw results.errors.join('\n');
//cc @johnjbarton

johnjbarton · 2014-07-10T22:03:32Z

I think we want the type throw to extend Error. We could have a CompileErrors with a .message which joins the .message from the results.errors. Ideally the error constructor name would also be included:

CompileErrors: Syntax Error: you messed up here
TypeError: and again here

Note that that require.js file is es5 so CompileErrors would need to be a function with the prototype dance, not a class. (All of this is why I was willing to settle with errors[0] ;-)

arv · 2014-07-10T22:08:44Z

src/node/require.js

  if (!results.js)
-    console.error(results.errors);
+    throw results.errors[0];


throw new Error(results.errors[0])

arv · 2014-07-10T22:09:36Z

Fixing our messy error reporting story is out of scope for this PR. We already have bugs about these.

With the change to the throw statement this looks good.

UltCombo · 2014-07-10T22:16:16Z

Thanks for the quick feedback guys! Fixed it as requested by @arv.

UltCombo · 2014-07-10T22:18:37Z

Oh, there's something weird. Now I'm at a loss again.

UltCombo · 2014-07-10T22:25:41Z

I've installed Make on my MSysGit but I'm still having issues to run the tests, by the way. The initial 2 tests run just fine, but then it throws:

Error: ENOENT, no such file or directory '<repository root>\test\commonjs-compiled'

Am I supposed to do anything other than npm install && npm test in order to run the tests?

johnjbarton · 2014-07-11T00:07:23Z

please create that directory and let me know if you hit any other problems.
I'll fix it .. or them ;-)

On Thu, Jul 10, 2014 at 3:25 PM, Fabrício Matté [email protected]
wrote:

I've installed Make on my MSysGit but I'm still having issues to run the
tests, by the way. The initial 2 tests run just fine, but then it throws:

Error: ENOENT, no such file or directory '\test\commonjs-compiled'

Am I supposed to do anything other than npm install && npm test in order
to run the tests?

—
Reply to this email directly or view it on GitHub
#1166 (comment)
.

UltCombo · 2014-07-11T00:38:27Z

@johnjbarton thanks, I've created commonjs-compiled, then I've created amd-compiled as well. Now I'm stuck with 3 ENOENTs:

amd Transpiled module export:
test/amd-compiled/NamedExports.js

context test compile module dir option AMD:
test/unit/node/resources/compile-amd/file.js

context test compile module dir option CommonJS:
test/unit/node/resources/compile-cjs/file.js

johnjbarton · 2014-07-11T01:46:09Z

I cloned your repo and built your branch without hitting any ENOENTs so I guess the problem here is related to windows. (The extra output is still there).

Please post the exact output. My guess is that we are not normalizing for windows; this may in fact be solved by PR #1155. You could just try that diff, its two lines.

UltCombo · 2014-07-11T01:53:39Z

@johnjbarton Well, those 3 failed test don't seem to affect the other tests, so don't give it too much thought. Well, I can open a separate issue if you'd like. To don't pollute this thread too much, I've posted the test script's output (stdout + stderr) on pastebin: http://pastebin.com/MMZJGCmm

I'm pondering why throw new Error() is being shown in the console while throw 'error message' is apparently not. Doesn't seem to make much sense to me, unless something is overriding the global Error constructor. edit: never mind, both print the error message to the console.

Well, I'm a bit too tired atm and got a full day tomorrow, so I might not be able to contribute very soon.

UltCombo · 2014-07-11T02:07:54Z

Oh wait, throw results.errors[0]; does print the error into my console, at least it is consistent.

UltCombo · 2014-07-11T02:32:43Z

Weirdly enough, if I use asyncFunctions for this test, then everything goes smoothly without any error output in the console. Guess this needs some more testing.

UltCombo · 2014-07-11T02:48:43Z

Looks like this an issue with Node.

Vanilla Node.js test case:

server.js (entry point):

try {
  require('./let-x');
} catch(e) {}

let-x.js:

let x = 21;

Live test case.

The error is logged when Node attempts to execute the code, even inside a try block.

I'm not sure if this is the expected behavior in this scenario. Assuming blockBinding: false, should Traceur still deem code containing let statements valid and pass it to Node? Or should Traceur itself throw before that? One way or another, I feel this should be a separate bug. For the current PR I can use another experimental feature such as asyncFunctions.

Thoughts?
//cc @johnjbarton @arv

UltCombo · 2014-07-11T03:02:54Z

To further prove my point, a fork of my test case above:

server.js (entry point):

console.log('Before let.');
try {
  require('./let-x'); // logs error to console
} catch(e) {
  console.log('inside catch');
}
console.log('After let.');

console.log('Before async function.');
try {
  require('./async'); // does *not* log any error!
} catch(e) {
  console.log('inside catch');
}
console.log('After async function.');

let-x.js

let x = 21;

async.js

async function foo() {}

Console output:

Before let.

/root/let-x.js:1
(function (exports, require, module, __filename, __dirname) { let x = 21;
                                                                  ^
inside catch
After let.
Before async function.
inside catch
After async function.

Live test case.

So, it is a Node.js issue after all.

UltCombo · 2014-07-11T03:20:13Z

I've reworked the unit test (blockBinding -> asyncFunctions) in order to avoid Node.js' peculiar behavior regarding the situation outlined above, let me know if there's anything I can improve.
/cc @johnjbarton @arv

johnjbarton · 2014-07-11T14:59:10Z

test/unit/node/require.js

+  test('traceurRequire.makeDefault options', function() {
+    var traceurRequire = require('../../../src/node/require'),
+        // TODO(arv): The path below is sucky...
+        fixturePath = path.join(__dirname, './resources/async-function.js'),


Sorry one more issue of style, this code base uses

var x; var y;

rather than

var x, y;

arv · 2014-07-11T16:11:07Z

Fixed in 66d05ab

UltCombo · 2014-07-11T17:28:22Z

@johnjbarton oh my bad, damn habits get the best of me. Thanks @arv for fixing it so quickly. =]

UltCombo · 2014-08-02T06:47:23Z

Just for the record, the extra output in the console was caused by this node bug: syntax errors are printed to stderr, even when wrapped with try/catch which has been fixed in the 0.11 branch by this PR.

UltCombo mentioned this pull request Jul 9, 2014

Fix Node API's options #1165

Closed

johnjbarton reviewed Jul 9, 2014
View reviewed changes

UltCombo changed the title ~~Node API: add options parameter to require() require.makeDefault()~~ Node API: add options parameter to require.makeDefault() Jul 9, 2014

arv reviewed Jul 10, 2014
View reviewed changes

src/node/require.js

if (!results.js)

console.error(results.errors);

throw results.errors[0];

Copy link

Collaborator

arv Jul 10, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

throw new Error(results.errors[0])

Node API: add options parameter to require.makeDefault(); Add author

b8c2818

johnjbarton reviewed Jul 11, 2014
View reviewed changes

arv closed this Jul 11, 2014

UltCombo deleted the node-require-makedefault-options branch July 11, 2014 17:28

johnjbarton mentioned this pull request Jul 14, 2014

Set default options for traceur.compile/traceur.require? #1162

Closed

Node API: add options parameter to require.makeDefault() #1166

Node API: add options parameter to require.makeDefault() #1166

Conversation

UltCombo commented Jul 9, 2014

UltCombo commented Jul 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnjbarton commented Jul 9, 2014

UltCombo commented Jul 9, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

UltCombo commented Jul 9, 2014

arv commented Jul 9, 2014

UltCombo commented Jul 9, 2014

UltCombo commented Jul 9, 2014

UltCombo commented Jul 9, 2014

arv commented Jul 9, 2014

arv commented Jul 9, 2014

UltCombo commented Jul 10, 2014

arv commented Jul 10, 2014

UltCombo commented Jul 10, 2014

arv commented Jul 10, 2014

UltCombo commented Jul 10, 2014

arv commented Jul 10, 2014

UltCombo commented Jul 10, 2014

arv commented Jul 10, 2014

johnjbarton commented Jul 10, 2014

UltCombo commented Jul 10, 2014

johnjbarton commented Jul 10, 2014

UltCombo commented Jul 10, 2014

UltCombo commented Jul 10, 2014

johnjbarton commented Jul 10, 2014

Choose a reason for hiding this comment

arv commented Jul 10, 2014

UltCombo commented Jul 10, 2014

UltCombo commented Jul 10, 2014

UltCombo commented Jul 10, 2014

johnjbarton commented Jul 11, 2014

UltCombo commented Jul 11, 2014

johnjbarton commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Jul 11, 2014

Choose a reason for hiding this comment

arv commented Jul 11, 2014

UltCombo commented Jul 11, 2014

UltCombo commented Aug 2, 2014