hasModule check fails if require is not defined

[iby]

I'm using webpack to build the bundle and after recent update moment.js gets incorrectly included into the module. Here is the full story: webpack/webpack#204 (comment).

This happens because it tries to check if the require is defined, I'm assuming this was done with a reason, but is it still valid these days?

hasModule = (typeof module !== 'undefined' && module.exports && typeof require !== 'undefined'),

Ideally this should be like this:

hasModule = typeof module !== 'undefined' && module.exports,

Or if the require check is beneficial for some reason, then perhaps like this:

hasModule = (typeof module !== 'undefined' && module.exports) || typeof require !== 'undefined',

[icambron]

The check for require was introduced in #1238 for QUnit, which I guess defines module and module.exports without defining require and for whatever we wanted to avoid using module.exports with it and just export moment as a global. I don't know enough about QUnit to know for sure, but I suspect the QUnit user simply shouldn't be using the global, and that they were simply working around its quasi-module implementation. So I'd be up for reverting that if @ichernev agrees.

[iby]

Does mr. @ichernev agree?

[ichernev]

@jgerigmeyer do you know why in qunit tests moment is used as a browser global. Also does QUnit have module.exports?
@icambron @ianbytchek I think what you say makes sense, but as you may imagine the js ecosystem is diverse enough so that any change would break somebody's workflow :)

So QUnit defines module and module.exports but is not CommonJS. For module -- ok, but why module.exports. I can not find this in the docs / source code.

[jgerigmeyer]

At the time I was using moment in a non-CommonJS/RequireJS project, and testing custom language settings via QUnit tests, which is why I was accessing moment as a browser global. (I've since decided unit tests don't need to test actual moment functionality, so I'm no longer doing this.)

QUnit itself does not define module.exports. This was a bug in an earlier version of sinon.js (see sinonjs/sinon#333).

In short, I agree that #1238 should probably be reverted.

I'm sure I'm being slow, but why is accessing moment via a browser global being discouraged?

[icambron]

It's not being discouraged in general. It's being discouraged in the presence of module, since that results in mucking with node global scope.

[jgerigmeyer]

Ah yes, I see that now. Makes perfect sense, #1238 should be reverted, and I apologize for not discovering the sinon.js bug before submitting that pull request 5 months ago.

[jzaefferer]

For the record, Qunit exports itself to module.exports since 1.13.0.

There has been an attempt to extend exports for more environments, but that isn't really going anywhere.

[ichernev]

Well we started from UMD (not really) and then patched it until every hostile environment and use case is supported. The state of the js module systems is just sad, very sad.

[icambron]

Closed by #1588