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

Fix check for a global variable "module" #49

Closed
wants to merge 1 commit into from

Conversation

timse
Copy link

@timse timse commented Apr 6, 2014

The simple check for module assumes that a global module variable exists, which is not the case if the browser just has an global exports variable and not a global module variable

also the tests on master currently fail (at least for me locally)

…sts, which is not the case if the browser just has an global exports variable and not a global module variable
@mroderick
Copy link
Owner

I have pushed changes to master that should make running the tests less dependent on your npm globals, and we're now also testing in both nodejs 0.8 and 0.10 on Travis.

I would suggest rebasing against master for a better experience :)

JSLint rejected your changes

src/pubsub.js:20:40 Unexpected 'typeof'. Use '===' to compare directly with undefined.

If you fix that, then I think all the tests should pass and we can merge your fix (thank you for contributing!)

@timse
Copy link
Author

timse commented Apr 15, 2014

they fail because jshint expects module to be a global variable that is always available (which is not the case) and thus asks me to compare it directly with undefined. but i cant since it doesnt exist :)

@mroderick
Copy link
Owner

they fail because jshint expects module to be a global variable that is always available (which is not the case) and thus asks me to compare it directly with undefined. but i cant since it doesnt exist :)

This project uses JSLint, not JSHint. The pubsub.js file has instructions for JSLint to ignore module

You can run JSLint with

$ grunt lint

# or this one, if you don't have grunt-cli installed as a global
$ $(npm bin)/grunt lint

@timse
Copy link
Author

timse commented Apr 26, 2014

same thing, you dont just tell JSLint to "ignore" the module you tell it that the module variable is an existing global variable. Which is just not the case all the time.
Thats why JSLint wants me to do a module !== undefined because it assumes that module exists but what i want to do with my check is to verify if module exists at all, assuming it may not, in which case the module !== undefined will throw ReferenceError: module is not defined.

If you still dont like it or dont understand what i'm trying to say here, just close it. This is getting too much of an effort.

@mroderick
Copy link
Owner

Thats why JSLint wants me to do a module !== undefined because it assumes that module exists but what i want to do with my check is to verify if module exists at all, assuming it may not, in which case the module !== undefined will throw ReferenceError: module is not defined.

Not exactly, JSLint tells you to use === to compare with undefined directly, and not with "undefined".

This code fails (on JSLint.com)

var module;
var exists = typeof module !== "undefined";

This code passes (on JSLint.com)

var module;
var exists = module !== undefined;

@timse
Copy link
Author

timse commented Apr 28, 2014

I'm seriously not sure if you are trolling me.

you define module var module;

and then OF COURSE you can do var exists = module !== undefined;

but in your code you cant just do var module; but you want to check if it exists, you cant initialize it localy because that would override the possible global one.

I give up, let your code break.

@mroderick
Copy link
Owner

I am not trolling you.

/*global module*/
var exists = module !== undefined;

@timse
Copy link
Author

timse commented Apr 28, 2014

just try it in your browser...

screen shot 2014-04-28 at 16 01 29

@timse
Copy link
Author

timse commented Apr 28, 2014

can we agree on removing exports/module from the globals list and access them via root that is given into the initialization function or whatever one may call it?

if (typeof root.exports === 'object' && root.module !== undefined){

@mroderick
Copy link
Owner

The scenario described in the initial post states

The simple check for module assumes that a global module variable exists, which is not the case if the browser just has an global exports variable and not a global module variable

If I understand this correctly, all modules using most of the UMD patterns would fail in that environment.

Would it help to switch to using commonjsStrictGlobal pattern to solve this issue?

@mroderick
Copy link
Owner

This has been fixed with #63

@mroderick mroderick closed this Aug 11, 2014
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.

2 participants