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

Simplify object check and eliminate lodash depdency #936

Merged
merged 4 commits into from
May 31, 2018

Conversation

matthargett
Copy link
Contributor

Mirrors reduxjs/redux#2599 . One tweak was to cache the original object prototype instead of calling the function twice.

@timdorr
Copy link
Member

timdorr commented Apr 25, 2018

This doesn't work because we don't have a isPlainObject.js locally.

@matthargett
Copy link
Contributor Author

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.

@timdorr
Copy link
Member

timdorr commented Apr 25, 2018

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 timdorr merged commit aba2452 into reduxjs:master May 31, 2018
@ajoshi0
Copy link

ajoshi0 commented Jun 12, 2018

@timdorr any updates on releasing this pr

@markerikson
Copy link
Contributor

@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.

@ajoshi0
Copy link

ajoshi0 commented Jun 13, 2018

@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

@markerikson
Copy link
Contributor

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'

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 ?

Copy link
Contributor

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.

Copy link
Member

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.

josepot pushed a commit to josepot/react-redux-lean that referenced this pull request Sep 21, 2018
* 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
albertodev7 pushed a commit to albertodev7/react-redux that referenced this pull request Dec 8, 2022
* 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
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

Successfully merging this pull request may close these issues.

5 participants