-
Notifications
You must be signed in to change notification settings - Fork 83
Removed dependency on lodash.assign #395
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
Conversation
var applyPolyfills = function() { | ||
if (typeof Object.assign !== 'function') { | ||
// Must be writable: true, enumerable: false, configurable: true | ||
Object.defineProperty(Object, "assign", { |
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 should not mutate global objects. I suggest to compare these 3 options and choose the one that results in smallest bundle size:
- Include this same helper function, but just call it if
Object.assign
isn't available, instead of mutating the globalObject
. - Use object-assign library from npm (https://www.npmjs.com/package/object-assign)
- Use TypeScript with object spread syntax
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.
Using typescript for only this looked like an overkill to me. I went for option 1 which will have no implact on the bundle size. I also modified all lodash.assignIn usages to use assign instead because it looks like we are not dealing with inherited properties here.
Summary
to reduce package size, we are trying to gradually get rid of lodash library. This PR replaces
assign
function withObject.assign
. Also added a polyfill method to support old browsers. Polyfill was taken from MDN.https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign#Polyfill
Test Plan
All tests pass after this change.