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.

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

Update has.js #101

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

Update has.js #101

Changes from 2 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
7 changes: 6 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,12 @@
// 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){
Copy link
Contributor

Choose a reason for hiding this comment

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

Consistent double quote use 'module' -> "module"

var moduleConfig = {};
if (typeof module.config === "function")
Copy link
Contributor

Choose a reason for hiding this comment

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

lets match the style:
==, no space between the if and the (, and use brackets {}.

Copy link
Author

Choose a reason for hiding this comment

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

It's ok to remove spaces but why do you want to use non-strict comparation for that? eqeqeq works faster and typeof always returns string

Copy link
Contributor

Choose a reason for hiding this comment

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

This code going to be hit a few million times is it? Naw, it's a micro-opt and counter to the existing style. Technically because typeof always returns a string the == is performing the same steps as ===.

moduleConfig = module.config() || {};
for (var propKey in moduleConfig)
Copy link
Contributor

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

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

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

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

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