1.3.0 breaks in Narwhal JS

[jdalton]

You all switched to only using module.exports for serverside JS.
That is a non-standard extensions supported by some, but not all serverside JS.
The previous version 1.2.0 worked fine.

[mathiasbynens]

I’m bumping into this issue when using jquery/qunit + jdalton/qunit-clib to run bestiejs/punycode.js unit tests. Please revert!

[jdalton]

Will also break in RingoJS < v0.8.0

[jzaefferer]

The previous version was exporting things twice. Is it enough to use extend(exports, QUnit); to make it work in both environments? Can someone help out with a patch that make it work as var QUnit = require('qunit'); in node.js, and whatever is used on Narwhal/Ringo?

[mathiasbynens]

For Punycode.js I used this pattern: https://github.com/bestiejs/punycode.js/blob/a6e30c4e2ce7a9a569bc2c84a3435bd5612be59f/punycode.js#L493-510 Thanks to @jdalton and @unscriptable.

Note that freeExports, freeModule, freeDefine, key etc. are declared at the top of the file.

[jzaefferer]

Any chance NarwhalJS will also implement what node.js and RingoJS 0.8 support?

[jdalton]

@jzaefferer It's trivial to support platforms like Narwhal by using the CommonJS standard exports object and not module.exports.

Simply change this:

// Expose the API as global variables, unless an 'exports'
// object exists, in that case we assume we're in CommonJS
if ( typeof exports === "undefined" || typeof require === "undefined" ) {
    extend(window, QUnit);
    window.QUnit = QUnit;
} else {
    module.exports = QUnit;
}

to this:

// Expose the API as global variables, unless an 'exports'
// object exists, in that case we assume we're in CommonJS
if ( typeof exports === "undefined" || typeof require === "undefined" ) {
    extend(window, QUnit);
    window.QUnit = QUnit;
} else if (typeof module === "object" && module.exports === exports) {
    // non-standard `module.exports` supported by Node.js and RingoJS 0.8.0+
    (module.exports = QUnit).QUnit = QUnit;
} else {
    exports.QUnit = QUnit;
}

The reason you do (module.exports = QUnit).QUnit = QUnit; is do devs can use the same require/assign pattern cross-enviro

var QUnit = require('./qunit').QUnit;

[jzaefferer]

Shouldn't that be var QUnit = require('./qunit')?

[jdalton]

Shouldn't that be var QUnit = require('./qunit')?

The reason you do (module.exports = QUnit).QUnit = QUnit; is so devs who have code that works in multiple enviros can just stick with 1 way to require the script. So if you are only using Node.js then ya you could just do var QUnit = require('./qunit'); but if you don't wan't to muck with it for multiple enviros then devs would have the option to just do var QUnit = require('./qunit').QUnit;.

[domenic]

extend(exports, QUnit); should work fine and make var QUnit = require('./qunit') work in both environments, I believe.

The only reason to use module.exports is if you're exporting a function.

[jdalton]

@domenic I would dig extend(exports, QUnit); too but it would have to be moved to the bottom of qunit.js. It's currently exported somewhere in the middle.

[jzaefferer]

Fixed by doing extend(exports, QUnit) and moving that the the end of the file.

[jdalton]

I take it back :P
extend(exports, QUnit) doesn't seem to work. It just hangs at the start.
Also you left the window export in the middle of the file. It too should be moved to the bottom.

[jzaefferer]

The window export is supposed to be half-way through, as we don't want to export all QUnit properties as variables.

Gonna install NarwhalJS to test it myself...

[jdalton]

@jzaefferer Thanks for digging into it, much appreciated.

[jzaefferer]

The export in NarwhalJS works fine, except when something goes wrong it chokes on jsDump's check for setInterval, or something like that. As so far no one reported that as an issue, I'm inclined to not care that much either. Specifically:

InternalError: Java class "[Ljava.lang.annotation.Annotation;" has no public instance field or method named "setInterval". (/Users/jza/dev/narwhal/packages/narwhal-lib/lib/narwhal/sandbox.js#118)

If you want to take a look, here's something to test with: d600c31

[jzaefferer]

Closing until someone who cares about narwhaljs can help out.