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

Confirm that require is a function #98

Merged
merged 1 commit into from Apr 6, 2017

Conversation

mrngoitall
Copy link
Contributor

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

@icambron
Copy link
Owner

icambron commented Apr 6, 2017

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

@mrngoitall
Copy link
Contributor Author

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

@icambron
Copy link
Owner

icambron commented Apr 6, 2017

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
Copy link
Contributor Author

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
Copy link
Owner

icambron commented Apr 6, 2017

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.

@icambron icambron merged commit 2c1de47 into icambron:master Apr 6, 2017
@mrngoitall mrngoitall deleted the check-require branch April 6, 2017 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants