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

[Core] Removes utils/extend.js #2933

Merged
merged 1 commit into from
Jan 14, 2016

Conversation

newoga
Copy link
Contributor

@newoga newoga commented Jan 14, 2016

This commit removes the extend.js utility. We already have enough existing merge alternatives such as our own style merger, lodash.merge, react-addon-update, Object.assign, and ES6 spread operators. I replaced each use of utils/extend with the "most appropriate" alternative according to code consistency and then removed utils/extend.js.

Note: Technically extend.js is a recursive merger. lodash.merge is also a recursive merger. Based on use cases, it seems like styles/theme-manager is the only case that actually requires recursive merging.

This commit removes the extend.js utility. We already have enough existing alternatives such as our ow style merger, lodash.merge, react-addon-update, Object.assign, and ES6 spread operators. I replaced the uses of utils/extend with the "most appropriate" replacements according to code consistency and then removed utils/extend.js

Signed-off-by: Neil Gabbadon <neil.gabbadon@emikra.com>
@oliviertassinari
Copy link
Member

@newoga Yeah! Let's get rid of this technical dept 🎉.

oliviertassinari added a commit that referenced this pull request Jan 14, 2016
@oliviertassinari oliviertassinari merged commit 386f3f4 into mui:master Jan 14, 2016
@mbrookes
Copy link
Member

"debt". Getting rid of the technical department is probably not what you want :winking:

@alitaheri
Copy link
Member

@newoga Thanks 😁

@oliviertassinari
Copy link
Member

@mbrookes I would rather be a native speaker 😁

@newoga newoga deleted the remove-utils-extend branch January 28, 2016 10:27
@igorbt
Copy link
Contributor

igorbt commented Apr 6, 2016

@newoga @oliviertassinari are you sure that this didn't break anything? Removed extend util was always returning a new object, without mutating the base (https://github.com/callemall/material-ui/blob/v0.14.2/src/utils/extend.js). lodash.merge is actually mutating the base (https://lodash.com/docs#merge).

@newoga
Copy link
Contributor Author

newoga commented Apr 6, 2016

@igorbt This code has gone through a few refactorings (for example neither Extend and mergeStyles() in this PR is being used in master at the moment). Currently Object.assign is being used and it is creating new objects when necessary. Could you open a new issue and describe the problem you're having in detail and what version you're having it in?

@zannager zannager added the core Infrastructure work going on behind the scenes label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants