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

Mention "bad practise" approach #75

Closed
markelog opened this issue May 30, 2016 · 6 comments
Closed

Mention "bad practise" approach #75

markelog opened this issue May 30, 2016 · 6 comments
Labels

Comments

@markelog
Copy link

Do i understand this correctly - such plugin should be used only if consumer doesn't use tools like rollup or webpack 2 where tree-shaking algorithm is implemented? If so, should this note be added to the docs?

Also, would you agree that this code -

import _ from 'lodash';

_.map([1, 2, 3], addOne);

Is worse then

import { map } from 'lodash';

map([1, 2, 3], addOne);

?

@markelog
Copy link
Author

markelog commented May 30, 2016

Is worse then

By that i mean "importing" the whole library (at least it is perceived that way) instead of one desired method, complicates understanding what methods is used from library and possible update to new tools in which this plugin wouldn't need to be used.

All and all such code doesn't look obvious to me, what do you think?

@jdalton
Copy link
Member

jdalton commented May 30, 2016

This plugin will transform code like:

import _ from 'lodash';

_.map([1, 2, 3], addOne);

as well as code like:

import { map } from 'lodash';

map([1, 2, 3], addOne);

Webpack2 and rollup do not support tree-shaking with Lodash.

@jdalton jdalton closed this as completed May 30, 2016
@markelog
Copy link
Author

Webpack2 and rollup do not support tree-shaking with Lodash.

Should this be mentioned in the docs? Do you have a relevant links?

as well as code like:

I know, but this is not what i asked

@jdalton
Copy link
Member

jdalton commented May 30, 2016

Should this be mentioned in the docs? Do you have a relevant links?

Folks tend to figure it out when they use them. There are open? issues on both I believe. Though the gist of the threads are shrug since both tree-shaking implementations are way too sensitive to potential side-effects, of which Lodash has none. Neither have seemed interested in addressing support for Lodash, so we've deferred to this plugin.

I know, but this is not what i asked

Lodash is pretty hands off when it comes to telling folks how to code. That's why we support both styles.

@markelog
Copy link
Author

markelog commented May 30, 2016

I guess you mean webpack/webpack#1750 and rollup/rollup#45?

I would add those links to readme, like "If you use rollup or webpack 2 this dependency will be no longer needed after following issues will be resolved - ..."

Lodash is pretty hands off when it comes to telling folks how to code.

I would be interested to hear how would you do this, not as recommendation, but your personal opinion, sorry if this out of the scope a bit

@jdalton
Copy link
Member

jdalton commented May 30, 2016

I would add those links to readme, like "If you use rollup or webpack 2 this dependency will be no longer needed after following issues will be resolved - ..."

I've gotten the impression that those issues won't likely be resolved.
I rather broken tools like webpack 2 and rollup link to us :P

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

No branches or pull requests

2 participants