-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Simplify object check and eliminate lodash depdency #936
Simplify object check and eliminate lodash depdency #936
Conversation
This doesn't work because we don't have a |
Of course I have to make a dumb mistake on my first contribution to this repo. This is what I get for pushing right before I'm rushing out the door to get my child 😆 I did run lint and test before pushing, I swear. Fixed. |
I swapped out the modified versions with their originals from Redux. Given the relative controversy over replacing lodash's version, I'd rather not stir the pot anymore beyond that 😄 |
@timdorr any updates on releasing this pr |
@ajoshi0 : whenever we have a reason to push out another release. This doesn't have any real user-facing impact, so I don't see what the urgency is to get out a release that has this change. |
@markerikson I just asked because My app bundle has 60+ KB bloat due to lodash, lodash-es which will be removed once you do a release but I will wait thanks |
Whatever's going on with your app, that is not due to React-Redux using Lodash-ES. Please check your own app's build setup. |
@@ -1,4 +1,4 @@ | |||
import isPlainObject from 'lodash/isPlainObject' | |||
import isPlainObject from './isPlainObject' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and I'm definitely a fan of removing the lodash dependency, but I wonder if instead of rolling our own isPlainObject we should bring in https://github.com/jonschlinkert/is-plain-object ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're happy with the version that Tim rolled while working on Redux 4.0. No reason to switch again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I rejected is-plain-object
because it uses the toString()
method, which is relatively expensive.
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 . * Add missing files. * Replace isPlainObject with unmodified version from Redux * Also replace the spec with the unmodified version
* Simplify object check and eliminate lodash depdency, mirroring reduxjs/redux#2599 . * Add missing files. * Replace isPlainObject with unmodified version from Redux * Also replace the spec with the unmodified version
Mirrors reduxjs/redux#2599 . One tweak was to cache the original object prototype instead of calling the function twice.