export.js: Export in all CommonJS environments

[bnjmnt4n]

Closes #521

[JamesMGreene]

This will throw an error if exports exists but is set to null.

From previous chats with @jdalton, I believe there are a few more variations necessary to thoroughly export for all CommonJS environments, none of which require a nodeType check AFAIR.

[jdalton]

none of which require a nodeType check AFAIR.

We ended up with nodeType check in our UMD because devs had used elements with the id exports or modules and browser auto expose them as globals.

[JamesMGreene]

@jdalton: Ah, makes sense. Thanks for the correction and explanation.

[jzaefferer]

This currently fails against jshint, which shouldn't be a big deal to address. The bigger issue though is the lack of actual tests. We had similar exports before I removed them in 2b9f0ee, since we had and still have no test coverage for those other "CommonJS" environments. Any improvement to exports must come with relevant tests that will prevent future regressions.

[bnjmnt4n]

How would you recommend the tests be done?

[jdalton]

I'll see what I can come up with, I use QUnit to test in a variety of cli environments. I bet we can come up with something that hooks into travis-ci as well.

[jzaefferer]

This is wrong.

[jzaefferer]

I've looked at current versions of UMD, and neither returnExports nor commonjsStrict use the pattern proposed here.

As to my comment "This is wrong" above: Nodejs is the one env we currently test against, and the usage there is pretty clear. Requiring qunit should return the QUnit object, not something that has a QUnit property.

[JamesMGreene]

As to my comment "This is wrong" above: Nodejs is the one env we currently test against, and the usage there is pretty clear. Requiring qunit should return the QUnit object, not something that has a QUnit property.

This is why we must use module.exports = QUnit; instead of exports.QUnit = QUnit; as you can't replace the whole exports object.

[bnjmnt4n]

Ok, I'll detect if module.exports is available and then use it, otherwise use exports instead.

[jdalton]

I explained the basic UMD steps I've done before here.

You can simulate the exports of other environments in node using its vm module.
See this and this.

[leobalter]

I would hold this till we have a patch to test it on the multiple target environments. Otherwise this would demand manual testing and create doubt on further changes on this piece of code.

It would be great to see QUnit on many environments, even though, we need to keep it reliable anywhere.

[bnjmnt4n]

@jdalton can you help me review this change? For some reason the tests are still failing, not sure what exactly I'm doing wrongly.

[jzaefferer]

• ReferenceError: test is not defined indicates a test runs without test being available as a global.
• missing name after . operator, assert.throws( looks narwhal chokes on throws?
• Rhino test seems to have the same problem with the missing test

Addressing the missing global should be reasonable. If you look at some of the code you removed, in the test-on-node task, it exported properties to node's global object. Restoring that code in some form should help.

I don't know what to do about the throws issue. Get them to fix their compiler?

[bnjmnt4n]

I'm trying to fix the issues about undefined test, but I can't do much about the narwhal issue. Perhaps you could use assert['throws'] instead?

[jdalton]

Search the issue tracker for throws to see several reported issues. Under ES3 throws is a reserved word and so some engines will throw errors when attempting to use it. I tackle this in qunit-extras by aliases throws as its old name raises:
QUnit.raises = QUnit['throws'] || QUnit.raises

[bnjmnt4n]

Running "test-on-rhino" task
Warning: ENOENT, no such file or directory� Used --force, continuing.
Running "test-on-ringo" task
Warning: ENOENT, no such file or directory� Used --force, continuing.
Running "test-on-narwhal" task
Warning: ENOENT, no such file or directory� Used --force, continuing.
Running "test-on-phantomjs" task
Warning: ENOENT, no such file or directory� Used --force, continuing.

[Krinkle]

@D10 I suspect this line might be the problem:

global.process.chdir( path.join(global.process.cwd(), "test") );

After the first one runs, it adds another /test to the path (leading to qunit/test/test/test/ etc.). Should probably avoid changing the cwd in geneal and instead include test/ in the filename passed to the program.

Ee.g. spawn( env, [ "test/env-tests.js" ] ) instead of spawn( env, [ "env-tests.js" ] ).

[bnjmnt4n]

Question: Is there a missing assignment of QUnit.config? Keep getting errors about QUnit.config being undefined.

[bnjmnt4n]

Fixed most of the issues already, but builds still fail on narwhal and phantomjs. @jdalton am I missing anything?

[Krinkle]

There should be nothing after done(), swap these lines. Unless it's this way on purpose, in which case I'm curious to learn why.

[bnjmnt4n]

It now works on Node, Narwhal, Rhino and Ringo, although Ringo doesn't log out the assertions. Not sure why...

[JamesMGreene]

@D10: How do you know it works on Ringo if the assertions aren't logged?

[bnjmnt4n]

You’re right, can’t test this now, maybe @jdalton can help when he’s free.

[bnjmnt4n]

Just tested on Ringo on my machine, it works, will try to debug. :)

[bnjmnt4n]

Travis doesn't seem to be testing new commits?

[leobalter]

Looks like the hook failed at some point. There's no known incident on Travis' status site.

[jzaefferer]

No more Narwhal, let's remove the reference.

[jzaefferer]

Finally did an actual code review. There's some stuff to address, but we're on a good way. Thank you @D10 for your effort.

Might be good to extend this with Nashorn support, but I think we can finish this first before looking into that env.

[bnjmnt4n]

Thanks for reviewing. Will amend commit and rebase on master this weekend.

[JamesMGreene]

Friendly ping for @D10. ☝️

[bnjmnt4n]

Sorry, I’ve been rather busy lately. Will try to update by this week.

[JamesMGreene]

We appreciate the effort, @D10. Of course, do make sure you rebase before doing your updates. 👍

[JamesMGreene]

Ping @D10. 💝

[bnjmnt4n]

Working on it, rebasing now ;) ❤️

[bnjmnt4n]

I've updated it, however it no longer works on Node, as src/exports.js and src/outro.js are not at the end of the generated file.

Additionally, I eventually intend to use packaged binaries for Rhino and Ringo on npm to simplify the install process.

[bnjmnt4n]

Ping @JamesMGreene @jzaefferer @leobalter

[leobalter]

We are close to solve this. Although, I would like to have an console reporter implemented as an option in the core, I'm not sure if the other collaborators agree with this, but I believe it's good to report to the log when HTML is not present.

Regarding this, we can implement a compatible reporter with those envs and use your contributions to run the tests on them.

[jzaefferer]

I still can't tell what the purpose of this is.

[bnjmnt4n]

For envs without module.export support, we need to export the whole QUnit object, so properties like QUnit.config can be accessed.

[jzaefferer]

I've extracted the discussion about raises into #663.

As for this PR itself and more importantly, the underlying issue #521: I've just tested rhino, ringo and nashorn by doing a simple load("dist/qunit.js") in each of them. In all three, a global QUnit is exported, along with all its properties, which can then be used to register a console reporter, write and run tests. Since that is the only reference needed to use QUnit, I can't tell why we would need any of this PR. #521 didn't specify which environment in particular wasn't working - I think I've covered the ones that qunit-extras supports.

Speaking of which, the addEventListener and setTimeout in qunit-extras are not needed anymore (at least with 1.15+). The excused feature could be replaced with QUnit.skip starting from 1.16 (not the same, but similar usecase). The rest might eventually get incorporated by a proper console reporter.

[jzaefferer]

#663 is fixed, 1.16 will ship with assert.raises as an alias for assert.throws.

Since there was no reaction to my last comment, I don't see anything else that we can do here.

[jdalton]

#521 didn't specify which environment in particular wasn't working - I think I've covered the ones that qunit-extras supports.

Looking at src/export.js you're missing the exports.QUnit = ... to support Rhino with the -require flag and (module.exports = QUnit).QUnit = ... for access consistency.

[jzaefferer]

@jdalton can you please create a regular bug report for that? Steps to reproduce mostly, otherwise we're stuck in the same place again.