I am trying to build a very basic JS framework for my own uses, ultimately collating a lot of pieces of JavaScript I have created over the years. But I would not call myself a JavaScript expert.
What I have written below works well enough but I really would like to know if I am making any fundamental errors in the way this is set up, rather than re-writing a lot of code further down the line.
This basic start point is mirroring the way jQuery can be written to select an element and chain an event to that object, but using all the querySelectorAll instead of the older methods.
If anyone can find basic or advanced flaws in what I've done that would be most useful.
<html xmlns="http://www.w3.org/1999/xhtml">
<head>
<script type="text/javascript">
///DO NOT COPY OR USE FOR COMMERCIAL PURPOSES
function Framework(sel) {
function myFramework(sel) {
///Add selections to array
this.sel = sel;
if (typeof this.sel !== "undefined") {
if (typeof this.sel !== 'string') {
this.selArray = new Array(this.sel);
} else {
this.what = this.sel.charAt(0);
if (this.what == ".") {
this.selArray = document.querySelectorAll(this.sel);
} else if (this.what == "[") {
this.selArray = document.querySelectorAll(this.sel);
} else if (this.what == "#") {
this.sel = this.sel.substr(1);
this.selArray = new Array(document.getElementById(this.sel));
}
}
}
myFramework.prototype.ready = function (func) {
document.onreadystatechange = function () {
var state = document.readyState
if (state == 'interactive') {
// init();
} else if (state == 'complete') {
func();
}
}
}
myFramework.prototype.click = function (func) {
for (i = 0; i < this.selArray.length; i++) {
if (window.addEventListener) {
this.selArray[i].addEventListener('click', func, false);
} else if (window.attachEvent) {
this.selArray[i].attachEvent('onclick', func);
}
}
return this;
}
return this;
}
return newTask = new myFramework(sel);
}
Framework().ready(function () {
Framework('.test').click(function () {
alert("it works!");
});
});
</script>
</head>
<body>
<div style="background-color:#FFFF00; height:10px" class="test"></div>
</body>
</html>
1 Answer 1
What you've got is not a framework. It is a library.
I can see you are basically creating a jQuery-like wrapper for selecting DOM nodes and event handling. Nothing terribly wrong with the main idea.
Why have a function called Framework which has a function inside of it? Every call to Framework(...) causes myFramework to be recreated. Instead, just use a regular prototype-based "class" in JavaScript:
function Framework(sel) {
if (!(this instanceof Framework)) {
// return a new Framework object if called without the "new" keyword
return new Framework(sel);
}
this.sel = sel;
if (this.sel) {
if (typeof this.sel !== 'string') {
this.selArray = new Array(this.sel);
} else {
this.what = this.sel.charAt(0);
if (this.what == ".") {
this.selArray = document.querySelectorAll(this.sel);
} else if (this.what == "[") {
this.selArray = document.querySelectorAll(this.sel);
} else if (this.what == "#") {
this.sel = this.sel.substr(1);
this.selArray = new Array(document.getElementById(this.sel));
}
}
}
}
Framework.prototype = {
constructor: Framework,
ready: function(func) {
// ...
},
click: function(func) {
// ...
}
};
The code that inspects the selector and calls querySelectorAll or getElementById is completely redundant and unneeded. If this.sel is a string, just blindly pass it to document.querySelectorAll. You've gained nothing by inspecting the selector. The speed difference between getElementById("foo") and querySelectorAll("#foo") will be negligible at worst, so why complicate your code?
if (this.sel) {
this.selArray = typeof this.sel === "string"
? document.querySelectorAll(this.sel)
: [this.sel];
}
Optimizing The Attaching Of Events
In your code, you loop over the array of elements, and on each iteration you test for attachEvent or addEventListener. Browsers won't magically change their native DOM API, so testing for these methods repeatedly when the result will be the same is extra work for the browser. Instead, create a method that does the event binding, which we will call bind.
Instead of testing for the "attachEvent" method on each iteration of the loop in click, test it once:
Framework.prototype = {
constructor: Framework,
ready: function(func) {
// ...
},
click: function(func) {
for (var i = 0; i < this.selArray.length; i++) {
this.bind(this.selArray[i], "click", func);
}
return this;
},
bind: !!document.addEventListener
? function(element, event, func) {
element.addEventListener(event, func, false);
return this;
}
: function(element, event, func) {
element.attachEvent(event, "on" + func);
return this;
}
};
The bind method is set on the prototype and makes use of a ternary operator to assign a function to the bind property that supports the current browser's native API instead of checking for support.
// The Test: Does the browser have a document.addEventListener method?
bind: !!document.addEventListener
// Yes: The browser does. Use this function to bind events using standard API
? function(element, event, func) {
element.addEventListener(event, func, false);
return this;
}
// No: The browse does not. Use Internet Explorer's API
: function(element, event, func) {
element.attachEvent(event, "on" + func);
return this;
}
The decision to use attachEvent or addEventListener is made once at page load. The Framework.prototype.bind method will directly use the supported API for each browser rather than test for it.
More information on ternary operators: Conditional (ternary) Operator
Implicitly Declared Global Variables
Don't forget that variable assignments without var before the variable name create implicitly declared global variables. The for loop in the click method does this with the i variable, and your original Framework method mysteriously declares a global variable newTask.
-
\$\begingroup\$ That's fantastic Greg, I didn't even know about that constructor syntax. \$\endgroup\$Guesser– Guesser2014年10月23日 21:23:38 +00:00Commented Oct 23, 2014 at 21:23
-
\$\begingroup\$ I'm a bit confused about the bind method for event listeners isn't this method being called in the loop anyway or does this have to do with memory management? \$\endgroup\$Guesser– Guesser2014年10月23日 21:24:44 +00:00Commented Oct 23, 2014 at 21:24
-
\$\begingroup\$ could you maybe explain this shorthand it's not something I recognise at least not the square brackets and line after the if statement... this.selArray = typeof this.sel === "string" ? document.querySelectorAll(this.sel) : [this.sel]; \$\endgroup\$Guesser– Guesser2014年10月23日 21:39:31 +00:00Commented Oct 23, 2014 at 21:39
-
\$\begingroup\$ Sure thing, Guesser. Let me update my answer. \$\endgroup\$Greg Burghardt– Greg Burghardt2014年10月24日 12:40:54 +00:00Commented Oct 24, 2014 at 12:40
-
\$\begingroup\$ Is calling the method "bind" not a bit confusing with this developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/… \$\endgroup\$Guesser– Guesser2014年10月27日 11:42:25 +00:00Commented Oct 27, 2014 at 11:42
///DO NOT COPY OR USE FOR COMMERCIAL PURPOSESYou do know that posting a question on a Stack Exchange is under cc by-sa 3.0. I am not a lawyer and don't know anything really about license or anything related to it, but I just wanted to make you aware of that fact. \$\endgroup\$