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

Replace lodash utils #131

Merged
merged 6 commits into from
Jun 18, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,7 @@
"dependencies": {
"babel-runtime": "^5.5.8",
"envify": "^3.4.0",
"invariant": "^2.0.0",
"lodash": "^3.9.3"
"invariant": "^2.0.0"
},
"browserify": {
"transform": [
Expand Down
4 changes: 2 additions & 2 deletions src/components/createConnector.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import identity from 'lodash/utility/identity';
import identity from '../utils/identity';
import shallowEqual from '../utils/shallowEqual';
import isPlainObject from 'lodash/lang/isPlainObject';
import isPlainObject from '../utils/isPlainObject';
import invariant from 'invariant';

export default function createConnector(React) {
Expand Down
2 changes: 1 addition & 1 deletion src/utils/composeStores.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import mapValues from '../utils/mapValues';
import pick from 'lodash/object/pick';
import pick from '../utils/pick';

export default function composeStores(stores) {
stores = pick(stores, (val) => typeof val === 'function');
Expand Down
3 changes: 3 additions & 0 deletions src/utils/identity.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function identity(value) {
return value;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually lodash's identity function is the same as yours but yes this does make sense to totally remove lodash as dependancy if this will be only one function needed from it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably best to put these all in a single file and export them individually that way you avoid the overhead of including each file, I think it adds ~3 lines and a bunch of bytes for every file you include.

This also has the benefit of keeping all the utils together.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an aside: identity would then be something like:

export const identity = x => x; which is just 😎

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@goatslacker Grouping these in one file sounds like a good idea to me however I worry it doesn't follow the style of the rest of utils of exporting one function per module (or as @taylorhakes mentions inlining some of them, especially simple ones like identity). Also, exporting arrow functions is pleasantly succinct but also another question of style :). @gaearon thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @goatslacker suggested, using an arrow function would be better because:

  • identity is not new-able
  • identity has no prototype

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably best to put these all in a single file and export them individually

👍

using an arrow function would be better

I think it's just a matter whether we want to have it as a named function or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's just a matter whether we want to have it as a named function or not.

It is not the only implication of arrow functions 😉

3 changes: 3 additions & 0 deletions src/utils/isPlainObject.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export default function isPlainObject(obj) {
return obj ? typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.prototype : false;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple way to detect a pojo is to do Object.prototype.toString.call(object) === '[object Object]' or something like that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I won't work:

var F = (function(){})
var f = new F
Object.prototype.toString.call(f) // [object Object]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ooflorent Are we trying to avoid the object above or Object.create(null)? It is perfectly fine for someone to have a React state that is not a plain object. ImmutableJS is used for state and it is not a plain object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is perfectly fine for someone to have a React state that is not a plain object.

I thought that broke in some recent version, didn't it? Either way they don't officially support it yet.
facebook/react#3303

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I want from isPlainObject:

  • no to arrays
  • no to null or undefined
  • no to ImmutableJS records (or other things that have a prototype)

I don't really care for edge cases like Object.create(null).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaearon I retract my earlier comment. You are correct

}
8 changes: 8 additions & 0 deletions src/utils/pick.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
export default function pick(obj, fn) {
return Object.keys(obj).reduce((result, key) => {
if (fn(obj[key])) {
result[key] = obj[key];
}
return result;
}, {});
}
11 changes: 11 additions & 0 deletions test/utils/identity.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import expect from 'expect';
import identity from '../../src/utils/identity';

describe('Utils', () => {
describe('identity', () => {
it('should return first argument passed to it', () => {
var test = { 'a': 1 };
expect(identity(test, 'test')).toBe(test);
});
});
});
19 changes: 19 additions & 0 deletions test/utils/isPlainObject.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import expect from 'expect';
import isPlainObject from '../../src/utils/isPlainObject';

describe('Utils', () => {
describe('isPlainObject', () => {
it('should return true only if plain object', () => {
function Test() {
this.prop = 1;
}

expect(isPlainObject(new Test())).toBe(false);
expect(isPlainObject(new Date())).toBe(false);
expect(isPlainObject([1, 2, 3])).toBe(false);
expect(isPlainObject(null)).toBe(false);
expect(isPlainObject()).toBe(false);
expect(isPlainObject({ 'x': 1, 'y': 2 })).toBe(true);
});
});
});
11 changes: 11 additions & 0 deletions test/utils/pick.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import expect from 'expect';
import pick from '../../src/utils/pick';

describe('Utils', () => {
describe('pick', () => {
it('should return object with picked values', () => {
const test = { 'name': 'lily', 'age': 20 };
expect(pick(test, x => typeof x === 'string')).toEqual({ 'name': 'lily' });
});
});
});