Conrad.js code review

review.md

Reviewing this version

Questions

Which browsers are targeted?

Would you consider making an AMD or UMD module instead of exporting to the global?

Comments

• L131. I feel you may get into random trouble if people use event names like valueOf, etc. Use Object.create(null) or a string-map lib
• L157. typeof arguments[0] === 'object' : null passes this test. You want ````Object(arguments[0]) === arguments[0]```
• L167-177 when you iterate over an array, use .forEach or one of its friends. In this case, I think you want .reduce (problem repeated in different places in the code)
• L205-206. Too much work :-p _handlers = Object.create(null) (recreate an empty object instead of cleaning it. It costs a bit more GC but... one object per _unbind call...)
• L261. This is tricky. The call may throw an error. You don't catch it, so one handler throwing prevents all other handlers from being called. It's unlikemy to be the expected behavior (add a comment if it's on purpose, though). Two possible solutions: try/catch and ignore the error or schedule each handler in its own setTimeout 0.
• L602. I wonder if a job shouldn't be an object with a kill/cancel method instead of the global killJob method.
• L620. It sounds like a lot of work to remove the job. Is the data structure right? Maybe killJob is called rarely enough that it doesn't matter?
• L738. _isRunning is an internal variable. You're in charge of making sure it remains a boolean, you shouldn't need to !!. TypeScript can help ;-)
• L893 - 949. I'm willing to bet Lodash has functions for this and does it better than you ever will. Just import the parts you need from Lodash
• L956 __dateNow: have you considered using performance.now?

Cosmetics

• L30: I believe the semilcolon isn't that important
• No need for the undefined in argument. It can't be overriden. That's an urban legend (or maybe true in super-old IEs?). If someone overides it, it can only be you by using the variable name, shadowing the global one. Just don't use this variable name ;-)
• I'm not sure the coding convention is necessary in the sense that if you export at the end, it's clear what's exported and consequently what isn't.
• .call(this) for an IIFE works, but the global object is then refered as this which is not an obvious way to reference it. I prefer:

(function(global){
   if(global.conrad){...}
})(this)

I feel global.conrad reads better than this.conrad

• L148. _bind: events is of type string, string[] should work too... or I don't know. Sometimes later, it's expected that it can be an object. Anyway, the doc doesn't match the code.
• L154. _bind: the use of this is awkward. what does it reference? (I guess the exported conrad reference, but I feel _bind would be better inlined or something)`
• L163. Object.prototype.toString.call(events) === '[object Array]' this is Array.isArray. Use it and polyfill if necessary. (happens in different places)

@jacomyal

I usually avoid Array helpers in libraries and use them only in applications, just to never deal with polyfills.

Hard call to make. These functions are standard and pretty useful, especially as they make the code more readable/idiomatic. But they cost something in transfer. I've never written/maintained a library, but I'd probably take the polyfill path (maybe with a separate file and build step).

Note that if people do try to polyfill these, it may break your code (you're iterating with for-in which visits the prototype). To work around this, either polyfill yourself :-p or add if(o.hasOwnProperty(prop)){...} verifications.