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

Consider dropping unnecessary Lodash dependency #19

Open
tdulcet opened this issue Aug 7, 2022 · 6 comments
Open

Consider dropping unnecessary Lodash dependency #19

tdulcet opened this issue Aug 7, 2022 · 6 comments

Comments

@tdulcet
Copy link

tdulcet commented Aug 7, 2022

Lodash should not be needed with modern JS. It also introduces a performance, maintenance and security cost on add-ons that use this template and your libraries. For these reasons, Firefox recently removed it from their codebase (see bug 1669968). Please consider doing the same for your libraries.

Also see this ESLint plugin: https://github.com/you-dont-need/You-Dont-Need-Lodash-Underscore

@rugk
Copy link
Member

rugk commented Aug 12, 2022

Well… I only had a limited amount of lodash libs to start with: https://github.com/TinyWebEx/AddonTemplate/tree/master/src/common/modules/lodash The intend indeed was what you say, to minimize the code base and avoid having all that hidden costs you mentioned.

They are manually picked/copied from the lodash lib.

And while I agree that getting rid of it would be great, I guess the only reason I have it in there is for debounce/throttle. The other files are just dependencies of these two libs.

And there are no replacements for that in ECMAScript as far as I know, so we cannot yet replace them,

@tdulcet
Copy link
Author

tdulcet commented Aug 14, 2022

And while I agree that getting rid of it would be great, I guess the only reason I have it in there is for debounce/throttle. The other files are just dependencies of these two libs.

Hmm, it looks like a lot more than just debounce/throttle is used:

$ grep -rni 'lodash' | grep 'import' | grep -v 'docs'
src/common/modules/AddonSettings/AddonSettings.js:11:import isPlainObject from "../lodash/isPlainObject.js";
src/common/modules/AddonSettings/tests/dataTest/defaultSettings.test.js:16:import isPlainObject from "../../../lodash/isPlainObject.js";
src/common/modules/AddonSettings/tests/helper/FakeStorage.js:1:import isPlainObject from "../../../lodash/isPlainObject.js";
src/common/modules/AddonSettings/tests/helper/FakeStorage.js:2:import isString from "../../../lodash/isString.js";
src/common/modules/AutomaticSettings/internal/LoadAndSave.js:13:import debounce from "../../lodash/debounce.js";
src/common/modules/Logger/Logger.js:17:import isPlainObject from "../lodash/isPlainObject.js";
src/common/modules/MessageHandler/CustomMessages.js:9:import isFunction from "../lodash/isFunction.js";
src/common/modules/RandomTips/RandomTips.js:12:import debounce from "../lodash/debounce.js";

and it seems throttle is actually never used. That use of debouce in AutomaticSettings is also currently undocumented (TinyWebEx/AutomaticSettings#4), so I suspect it is not used in practice.

And there are no replacements for that in ECMAScript as far as I know, so we cannot yet replace them

They should be able to be replaced with standard setTimeout() or requestAnimationFrame(), which is what Lodash is using internally. There is a simple implementation of debounce here using setTimeout() that should be able to handle your use cases.

@rugk
Copy link
Member

rugk commented Aug 19, 2022

Okay interesting. Though note throttle etc. is used in the actual add-ons etc.

And thanks for the "you might not need" website. Though that actually still argues:

But you should use Lodash.

It’s a great library, well crafted, battle tested and with a very skilled and active community contributing. The goal of this project is NOT to provide drop in replacements, but to show how achieve similar functionalities in plain Javascript, to understand how things work behind the hood.

And as you say, it's a tested implementation that does the same, essentially.

@tdulcet
Copy link
Author

tdulcet commented Aug 20, 2022

Though note throttle etc. is used in the actual templates etc.

I am not sure what you mean, as I searched the Awesome Emoji Picker, Unicodify and this template and could not find any uses of throttle.

And as you say, it's a tested implementation that does the same, essentially.

Well, it would allow you to drop the unnecessary Lodash dependency and replace 11 JS files (and hundreds of lines of code) with just 12 lines. IMO, that would be a huge win for performance, maintainability and security. It would also further the goal of your organization of making extensions "tiny".

@rugk
Copy link
Member

rugk commented Aug 22, 2022

Okay, and I also see it is being tested upstream there, so if we really hardly use it, then yeah… you've convinced me.

Any PR to replace that lodash throttle with "you might not need" throttle are fine. (And removing all the other stuff hmm...)

@rugk
Copy link
Member

rugk commented Aug 22, 2022

Also remove the doc section on lodash then: https://github.com/TinyWebEx/common#third-party-dependencies

Also I'm moving this to TinyWebEx Common as it affects all extensions.

@rugk rugk transferred this issue from TinyWebEx/AddonTemplate Aug 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants