Update has.js #101
Update has.js #101
Conversation
Adding requirejs config support
Added condition to extract plugin config
| define("has", function(){ | ||
| define("has", ['module'], function(module){ | ||
| var moduleConfig = {}; | ||
| if (typeof module.config === "function") |
There was a problem hiding this comment.
lets match the style:
==, no space between the if and the (, and use brackets {}.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ===.
|
ok, I removed third '=' |
| if(typeof module.config == "function"){ | ||
| moduleConfig = module.config() || {}; | ||
| } | ||
| for (var propKey in moduleConfig) |
There was a problem hiding this comment.
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){There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
| if(typeof module.config == "function"){ | ||
| moduleConfig = module.config(); | ||
| } | ||
| for (propKey in moduleConfig) |
There was a problem hiding this comment.
Soo close! Brackets + spacing of the for(
|
😄 what about now? |
|
Can you add a unit test and have you signed the CLA? |
| @@ -149,7 +149,14 @@ | |||
| // 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 propKey, moduleConfig = {}; | |||
There was a problem hiding this comment.
no need to init moduleConfig = {}; it can just be moduleConfig;.
|
alright. I've signed it just now 😀 |
|
CLA submitted on 2014-06-05 21:06:04. |
|
So for the test it looks like runTests.html has |
|
omg! the version of requirejs is 0.14.5+ |
|
+1 |
|
I come up with that approach for tests or, we can rewrite all detects to use requirejs (: |
|
I'll try to review this soon but can't make any promises 😞 |
|
This waits whole month for review 😒 |
|
Sorry, I haven't made time for this yet. |
|
Howdy @jdalton! So, what's about this? |
|
I gotta be honest, I'm not really an active contributor to this project anymore. If any other contributor has time to pick this up, review, & merge please have at it. |
|
I see. Alright, let's put this on their decisions :) |
Adding requirejs config support