Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build: Support Node.js export parity with CommonJS #709

Merged
merged 1 commit into from
Jan 9, 2015
Merged

Build: Support Node.js export parity with CommonJS #709

merged 1 commit into from
Jan 9, 2015

Conversation

JamesMGreene
Copy link
Member

Ref #521
More info: #521 (comment)

Basically, we want to ensure that require('qunitjs').QUnit works in all CommonJS environments, even if it is achieved by QUnit.QUnit = QUnit. Node.js was the only outlier at this point.

Also added and updated a few related tests.

QUnit.test( "QUnit exports", function( assert ) {
var qunit = require( "../dist/qunit" );

assert.ok( qunit, "Required module QUnit truthy" );
Copy link
Member

Choose a reason for hiding this comment

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

This might be discarded regarding the following test.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed one of the following assertions but I intend to keep this one simple assertion.

@jzaefferer
Copy link
Member

@JamesMGreene could you update this?

@JamesMGreene
Copy link
Member Author

Updated. Added another commit to address a few of the PR feedback comments. I have no rebased it yet, just for the sake of seeing the changes to the 1st commit and the comment history. I'll happily rebase it whenever we think it's ready to be merged.

@JamesMGreene
Copy link
Member Author

Eh... the commits were small enough, so I went ahead and rebased against master now. This way, the PR can be easily merged even if I am unresponsive for a bit.

My 2nd commit (now just rebased into the primary commit) was just removing 2 assertions (and 1 test) related to the now-irrelevant QUnit.constructor.

@jzaefferer
Copy link
Member

Looks good to me.

@leobalter leobalter merged commit 56279ad into qunitjs:master Jan 9, 2015
@leobalter
Copy link
Member

merged

@JamesMGreene JamesMGreene deleted the node_exports_fix branch January 9, 2015 18:26
@JamesMGreene
Copy link
Member Author

P.S. @jdalton Sorry, forgot to tag you on this earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants