search

icambron / twix.js

:hourglass::left_right_arrow: A date range plugin for moment.js
https://isaaccambron.com/twix.js/
MIT License
379 stars 54 forks source link

Confirm that require is a function #98

Closed mrngoitall closed 7 years ago

mrngoitall commented 7 years ago

Checks if require is actually a function available to us before we use it to MakeTwix.

icambron commented 7 years ago

What issue does this fix? You had exports but not require?

mrngoitall commented 7 years ago

Yeah, I had PhantomJS giving me an error about not having require when it tried running tests.

icambron commented 7 years ago

I don't really get it; seems like it should only call require() if your tests use require, in which case...it should exist. I pulled down phantom.js and checked what it defines:

phantomjs> global.module
undefined
phantomjs> typeof(global.require)
"function"

To be sure, I have no idea what that require function actually does but I don't really understand how you got in the spot where module was defined but require was something else. Can you elaborate?

I'm not trying to be obstructive here; it's just that I've been burned in the past making changes to loading code that I didn't really understand that fixed environment X but broke environment Y.

mrngoitall commented 7 years ago

I'm not 100% certain what conditions lead to phantomjs having access to module.exports without require, but I'm guessing it has something to do with not having the requirejs framework configured in karma (I'll have to play around with it to verify that tomorrow).

Either way, this change shouldn't have any impacts on another build environments, considering all it's doing is making sure that require is actually available as a function rather than assuming that it is a function and trying to execute it. We're just adding an extra safety net to capture this particular scenario. If things are already working for other people, it should continue to work, since typeof(require) should be a function already.

icambron commented 7 years ago

I'm going to merge this since I agree it's harmless. But I suspect you'll want to find out what's defining module.exports but not require on your end.