-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Remove lodash as a production dependency #1122
Remove lodash as a production dependency #1122
Conversation
@calebmer: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/ |
@@ -435,7 +432,7 @@ export default class ApolloClient { | |||
} | |||
|
|||
// ensure existing store has apolloReducer | |||
if (isUndefined(reduxRootSelector(store.getState()))) { | |||
if (reduxRootSelector(store.getState()) === undefined) { |
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.
The better way to do this is typeof x === 'undefined' right? Since undefined isn't a keyword and could be reassigned.
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.
Yes, that's right in theory. I wonder if it's ever made a difference for anyone in practice?
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.
IDK maybe it's just one of those old JavaScript trivia things that nobody cares about anymore then.
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.
I think it's still a valid thing to change. The only reason I didn't do that in the first place is because lodash implements it this way: https://github.com/lodash/lodash/blob/4.17.4/lodash.js#L12212
…but they do also have this which I missed: https://github.com/lodash/lodash/blob/4.17.4/lodash.js#L12
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.
oh, interesting (L12). I wonder why they did that instead of using typeof.
a816598
to
8996670
Compare
Great stuff @calebmer! I'd say that's enough low-hanging fruit so we can close the other issue. Except maybe that we want to get rid of whatwg-fetch as well for other reasons. |
Gosh guys this is incredible. Great work @calebmer! |
BTW other low-hanging fruit: getFromAST and storeUtils are duplicated in apollo-client and graphql-anywhere both. |
Removes
lodash
as a production dependency to reduce size. Many of the utilities were inlined such asisString
andisNumber
. For more involved utilities, I created a smaller version insrc/util
and added tests. These utilities includeisEqual
,cloneDeep
, andassign
. The smaller version insrc/util
do not cover all the edge cases that there Lodash equivalents cover. Most notably they do not handle circular objects gracefully (let me know if this is important).Removing Lodash takes us from 136 kB to 94 kB ungzipped and 35.3 kB to 24.5 kB gzipped. Roughly a 30% decrease in size 🎉