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

Refactor/depends on lodash #51

Merged
merged 4 commits into from
Feb 9, 2017
Merged

Refactor/depends on lodash #51

merged 4 commits into from
Feb 9, 2017

Conversation

tomchentw
Copy link
Contributor

From #22 (comment)
@JaKXz instead of replacing lodash modular dependencies, would you consider switching dependencies to lodash main module and import from its entries? For example,

Before

import isPlainObject from 'lodash.isplainobject';
import isString from 'lodash.isstring';
import isSymbol from 'lodash.issymbol';

After

import isPlainObject from 'lodash/isPlainObject';
import isString from 'lodash/isString';
import isSymbol from 'lodash/isSymbol';

Thus, for those users who's also using lodash in their user space, the bundler (rollup/webpack) could resolve these modules and create a smaller output.

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 93824e8 on tomchentw:refactor/depends-on-lodash into f787b9d on acdlite:master.

Copy link
Contributor

@JaKXz JaKXz left a comment

Choose a reason for hiding this comment

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

@tomchentw thanks! though I still prefer what you originally suggested with lodash/isString etc.

Is there any reason you didn't use that [or am I missing something about how babel-plugin-lodash works]?

@tomchentw
Copy link
Contributor Author

@JaKXz that's how it looks like when running yarn run build:

screen shot 2017-02-08 at 12 11 43 pm

@JaKXz
Copy link
Contributor

JaKXz commented Feb 8, 2017

@tomchentw oh ok, cool. Would destructuring those methods out of lodash also produce that result?

@tomchentw
Copy link
Contributor Author

@JaKXz I think so! Would you like switching to destructing format?

screen shot 2017-02-08 at 3 27 36 pm

@JaKXz
Copy link
Contributor

JaKXz commented Feb 8, 2017

Yes, please :)

@tomchentw
Copy link
Contributor Author

@JaKXz updated!

@coveralls
Copy link

coveralls commented Feb 9, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a299fde on tomchentw:refactor/depends-on-lodash into 01e8fc3 on acdlite:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling a299fde on tomchentw:refactor/depends-on-lodash into 01e8fc3 on acdlite:master.

@JaKXz JaKXz merged commit 5eb76c5 into redux-utilities:master Feb 9, 2017
@JaKXz
Copy link
Contributor

JaKXz commented Feb 9, 2017

Thanks @tomchentw! I'm going to bed now but I will release this by the end of the week when I have OSS cycles again :)

In the meanwhile please feel free to install flux-standard-action#master and give it a whirl.

@tomchentw tomchentw deleted the refactor/depends-on-lodash branch February 9, 2017 07:03
@JaKXz
Copy link
Contributor

JaKXz commented Feb 24, 2017

+ [email protected] finally published! sorry for the delay!

@tomchentw
Copy link
Contributor Author

Awesome!

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.

3 participants