Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

chore(lodash): require specific lodash/assign module #196

Merged
merged 4 commits into from
Jul 7, 2017
Merged

chore(lodash): require specific lodash/assign module #196

merged 4 commits into from
Jul 7, 2017

Conversation

WesSouza
Copy link
Contributor

Importing from lodash loads the entire lib into memory unnecessarily.

Importing from `lodash` loads the entire lib into memory unnecessarily.
@apollo-cla
Copy link

@wesleydesouza: 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/

@WesSouza
Copy link
Contributor Author

It seems the build fail is not related to this PR changes.

@mistic
Copy link
Contributor

mistic commented Jul 3, 2017

@wesleydesouza I think my pr should fix this build fail on your pr :)

Copy link
Contributor

@mistic mistic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we are only using webpack for the browser compile
this is useful 👍

@WesSouza
Copy link
Contributor Author

WesSouza commented Jul 6, 2017

@mistic yeah, for sure the browser build is now massively smaller (~83 kB).

And it seems the intended package was in fact lodash.assign, the previous require will fail if no library depends on lodash (which is fine for people that use webpack since async depends on lodash 😛 )

I've switched to that, but it seems I've done something wrong with the type declarations and test is failing again, I'm not very familiar with TypeScript.

Done, first time I see import x from 'y' versus import x = require('y')... hehe

Also introduce a small fix on src/test/tests.ts.
@dotansimha
Copy link
Contributor

@wesleydesouza why changing import to require ? Your typings looks fine and I think you can use import here.

@dotansimha dotansimha merged commit c09dec7 into apollographql:master Jul 7, 2017
@dotansimha
Copy link
Contributor

@wesleydesouza never mind, got it :)

Thanks @wesleydesouza !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants