-
Notifications
You must be signed in to change notification settings - Fork 99
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
chore: remove lodash #164
base: main
Are you sure you want to change the base?
chore: remove lodash #164
Conversation
…gin-Spatial-Navigation into chore/remove-lodash
Wrote an indentical set of tests, one that ran against lodash utils and one that ran against our own utils. Made changes to our own utils to ensure that the correct outcome was obtained
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @christianEconify,
Thanks again for opening the PR.
We had some discussions around this internally now, as well as double- (and triple-checked) how it is currently working.
A couple of things should be pointed out and discussed here, before we consider this or any related changes -
- If the primary motivator for this PR is related to issues with tree shaking, then this is entirely driven by the host application, as per how lodash is referenced and bundled: https://github.com/NoriginMedia/Norigin-Spatial-Navigation/blob/main/webpack.config.prod.js#L19
From what we can see when the tree-shaking is working as it should, only the intended lodash dependencies are required in the final bundle:
- lodash/debounce
- lodash/difference
- lodash/filter
- lodash/findKey
- lodash/first
- lodash/forEach
- lodash/forOwn
- lodash/noop
- lodash/sortBy
- lodash/throttle
- lodash/uniqueId
- The current lodash imports are from trusted, well maintained and battle-tested sources
We have been using these lodash imports for many years now, in the current cherry-picked form since at least 2019: https://github.com/NoriginMedia/react-spatial-navigation/releases/tag/v1.0.2
(As well as this being something we do not need to manually maintain ourselves. ➕)
The proposed replacement utils.ts would be a couple of kilobytes lighter in the end, which is of course a good thing, but there are a couple of other discrepancies to note when comparing these two as well:
difference
- Doesn't use any deep equality checks (like lodash.difference does), and it only seems support string and numbers types, so its more "targeted"
findKey
- Directly calls Object.keys().find(), which is generally an optimization
debounce
andthrottle
- Similar functionality, but lodash handles more edge-cases than the suggested functions do -
F.ex.debounce
correctly calls the function immediately withleading: true
, but it doesn't reset the timeout when invoked immediately. This means that if the function is called multiple times right after the initial call it would not delay properly. The current lodashdebounce
version ensures subsequent calls adhere to the wait period and don't fire immediately.
- Similar functionality, but lodash handles more edge-cases than the suggested functions do -
That said we do understand the motivation for looking into removing lodash, especially considering bundle size constraints when shipping to TVs.
While the proposed implementation does reduce the bundle size a bit, it also introduces a couple of trade-offs, initially in terms of functionalities and edge-case handling, but perhaps more importantly the long-term maintainability of this project.
As such, it's a bit difficult to provide a definitive yes or no on whether we should proceed with this change.
@christianEconify: Fair points. We have discussed this again today and we're still a bit unsure, to be entirely honest, but I am doing my best to see if we can find a good way forward with this. The main reasoning for moving away from lodash is completely fair, but if the client (host) app that uses this library is already depending on lodash itself, then this will in the end result in a doubling of a lot of the same utilities. (Technically this is the case today, if the host app has additional (equivalent or similar) "custom utilities", or even something like underscore for that matter, the outcome would still be a doubling of the "same" code.) It's almost tempting to expose an alias / interface where you could supply your "variant" of these functions yourself, be it through lodash functions or your own. We'll get back to you once I've had time to go over this a bit more and get closer to a proper answer. |
Removes lodash and replaces with internal functions, or regular javascript APIs
Have tested on internal projects and everything seems to work okay so far, however plan on more QA.
I think it's a good idea to get this PR open early for feedback