search

gajus / react-youtube-player

React component that encapsulates YouTube IFrame Player API and exposes player controls using the component properties.
Other
40 stars 17 forks source link

rm lodash dep when not necessary #4

Closed akofman closed 8 years ago

akofman commented 8 years ago

Thanks for this useful lib ! I just removed lodash from it because it seems to be overkill to just check a string.

gajus commented 8 years ago

Thanks to https://github.com/gajus/babel-plugin-lodash-modularize only the relevant bits of lodash are included in the build, e.g. https://github.com/gajus/react-youtube-player/blob/5e605c6d7dd3a2b243ce9542d289842f725967b2/dist/index.js#L3

jide commented 8 years ago

What's the point of including a lib that can be replaced with simple code ? Problem is that even if the dist is correct, the package.json will be used by npm in projects including it, thus it will actually install whole lodash and do some mess with dependency graph etc on the project although it's unneeded. At the minimum it should be in devDeps then.

gajus commented 8 years ago

What's the point of including a lib that can be replaced with simple code ?

Declarative programming.

Problem is that even if the dist is correct, the package.json will be used by npm in projects including it, thus it will actually install whole lodash and do some mess with dependency graph etc on the project although it's unneeded.

If your entire dependency tree does not have lodash in it, chances are – you are doing something wrong.

At the minimum it should be in devDeps then.

This suggestion makes no sense.

bitspook commented 8 years ago

If your entire dependency tree does not have lodash in it, chances are – you are doing something wrong.

There are better alternatives for a tool-belt library (if that's what you meant).

Just saying. Not that I have any problem with this package including lodash.

gajus commented 8 years ago

Let me rephrase that:

If you are modelling your dependencies depending on their dependence on a particular library (just because how it affects the install time), then you are doing something wrong.

jide commented 8 years ago

Okay so we have an npm package that depends on lodash. But in reality it does not depend on lodash since its main points to a compiled file where lodash is not used.

Wouldn't it make sense to move the dep in devDeps then ? It's used only for development, that is compiling the dist. In production it's not needed.

Or am i missing something here ?

jide commented 8 years ago

Arg, sorry okay, the lodash function is not in the code, my bad, I thought babel would include it.

akofman commented 8 years ago

Hey @gajus ! Indeed I didn't see that https://github.com/gajus/babel-plugin-lodash-modularize ! :+1: Sorry for the noise.