Khan / style-guides

Docs for the Organization
2.14k stars 147 forks source link

underscore comparison nits #43

Open jdalton opened 8 years ago

jdalton commented 8 years ago

Sorry for the rapid fire comments, needed to get it out before jumping into a meeting and forgetting. Continuing the feedback from #28.

There are a couple of methods that are sufficiently complicated and don't have a direct equivalent so instead we have a custom-built copy of lodash containing only those specific methods. You can find this file at: third_party/javascript-khansrc/lodash/lodash.js along with instructions on how to build it and exactly what methods are included.

In addition, Lodash is totally modular & works great with browserify / webpack to create smaller bundles. There's even a babel plugin to avoid mucking with module paths.

var cloneDeep = require('lodash.clonedeep');
// or
var cloneDeep = require('lodash/lang/cloneDeep');

Object.assign(json, this.model.toJSON()) | _.extend(json, this.model.toJSON())

It would be better to compare to _.assign as that's closer to Object.assign (iterating only own properties of source values instead of own+inherited in _.extend).

for (const [key, val] of Object.entries(obj)) {} | _.each(obj, fn)

In Lodash there's _.forOwn and _.forIn for object iteration too

defer | setTimeout(fn, 0); | _.defer(fn); delay | setTimeout(fn, 2000); | _.delay(fn, 2000);

The useful bit for _.defer and _.delay is they allow partially applying arguments to the deferred/delayed function, which older environments lacked in setTimeout (may be worth noting).

bindAll | `obj.method = obj.method.bind(someObj);

Typo, fix to obj.method = obj.method.bind(obj);

once

{
    method: () => {
        if (this._initDone) { return; }
        this._initDone = true;
        ...
}

This will assign _initDone to the outer this so not really appropriate for per method state.

csilvers commented 8 years ago

Thank you for your very thorough feedback! I'll let John (Resig) speak to the particulars, since he's a lot more familiar with the technical domain than I am.

One thing that may help put this chart in context: it's intended for Khan Academy employees when writing/modernizing KA code. So the "instead of" table refers to underscore constructs that are actually used in KA code. It's not meant to be an exhaustive list of all functions that exist in underscore or lodash.

jdalton commented 8 years ago

One thing that may help put this chart in context: it's intended for Khan Academy employees when writing/modernizing KA code.

Yep, that's cool. Most of my comments can be seen as just adding extra info. Besides the typos or incorrect usage the rest can be taken as is (no pressure to change bits) but if something is considered notable then by all means note it up :)