Replace lodash utils #131
Replace lodash utils #131
Conversation
| @@ -0,0 +1,3 @@ | |||
| export default function identity(value) { | |||
| return value; | |||
| } | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
As an aside: identity would then be something like:
export const identity = x => x; which is just 😎
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
As @goatslacker suggested, using an arrow function would be better because:
identityis notnew-ableidentityhas noprototype
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😉
Just returning the argument so can check by reference instead of deep equal
|
These functions are only used in one spot each. I think it would be better to keep the code inline and not create an unnecessary abstraction in another file. If they start to get used a lot or become a source of bugs, then put them in another file. |
| @@ -0,0 +1,3 @@ | |||
| export default function isPlainObject(obj) { | |||
| return typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.prototype; | |||
There was a problem hiding this comment.
This one does not work.
isPlainObject(null) // TypeError: Object.getPrototypeOf called on non-objectThere was a problem hiding this comment.
Thank you, added another test case for calling with null
| @@ -0,0 +1,3 @@ | |||
| export default function isPlainObject(obj) { | |||
| return obj ? typeof obj === 'object' && Object.getPrototypeOf(obj) === Object.prototype : false; | |||
There was a problem hiding this comment.
A simple way to detect a pojo is to do Object.prototype.toString.call(object) === '[object Object]' or something like that
There was a problem hiding this comment.
I won't work:
var F = (function(){})
var f = new F
Object.prototype.toString.call(f) // [object Object]
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
@gaearon I retract my earlier comment. You are correct
|
This looks good to me. I want to go ahead with #112 so merging. |
From #112 -- Removing lodash utils decreases the build size significantly as described by @dariocravero so this PR replaces the remaining three lodash modules with basic utils.