Skip to content

Update has.js #101

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.

Sign up for GitHub

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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Update has.js #101

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
40b2be2
Update has.js
AndrewEastwood Jun 3, 2014
86fd9d8
Update has.js
AndrewEastwood Jun 5, 2014
704aedc
Update has.js
AndrewEastwood Jun 5, 2014
f269bbe
Update has.js
AndrewEastwood Jun 5, 2014
81248c0
Update has.js
AndrewEastwood Jun 5, 2014
86b810a
Update has.js
AndrewEastwood Jun 5, 2014
04a4553
Update has.js
AndrewEastwood Jun 5, 2014
5ff1ca9
updating tests
AndrewEastwood Jun 6, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion has.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,13 @@
// Expose has()
// some AMD build optimizers, like r.js, check for specific condition patterns like the following:
if(typeof define == "function" && typeof define.amd == "object" && define.amd){
define("has", function(){
define("has", ["module"], function(module){
var moduleConfig = {};
if(typeof module.config == "function"){
moduleConfig = module.config() || {};
}
for (var propKey in moduleConfig)
Copy link
Contributor

@jdalton jdalton Jun 5, 2014

Choose a reason for hiding this comment

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

Undefined values in a for-in should not error,e.g. for(var foo in undefined){ so no need for all the object guards.

Copy link
Author

@AndrewEastwood AndrewEastwood Jun 5, 2014

Choose a reason for hiding this comment

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

moduleConfig can't be undefined because if somehow module.config() returns value that is null or undefined then {} will be assigned to moduleConfig

Copy link
Contributor

@jdalton jdalton Jun 5, 2014

Choose a reason for hiding this comment

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

The code style seems to favor hoisting vars out (it was a phase :P). So that would make it:

var propKey, moduleConfig;
if(typeof module.config == "function"){
  moduleConfig = module.config();
}
for(propKey in moduleConfig){

Copy link
Author

@AndrewEastwood AndrewEastwood Jun 5, 2014

Choose a reason for hiding this comment

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

What if we just use simple code for this as @jrburke has done in https://github.com/requirejs/text/blob/master/text.js#L23

var moduleConfig = (module.config && module.config()) || {};
for(var propKey in moduleConfig){
    has.add(propKey, moduleConfig[propKey]);
}

something like that above

Copy link
Contributor

@jdalton jdalton Jun 5, 2014

Choose a reason for hiding this comment

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

Naw, different project styles. We are consuming a foreign thing so have to be more cautious whereas that plugin expects to work with RequireJS and its constructs.

has.add(propKey, moduleConfig[propKey]);
return has;
});
}
Expand Down