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.

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

underscore comparison nits #43

Open
jdalton opened this issue Oct 30, 2015 · 2 comments
Open

underscore comparison nits #43

jdalton opened this issue Oct 30, 2015 · 2 comments

Comments

@jdalton
Copy link

jdalton commented Oct 30, 2015

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
Copy link
Member

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
Copy link
Author

jdalton commented Oct 30, 2015

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants