-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: changed from lodash to es-toolkit #18162
Conversation
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -13,7 +13,7 @@ import React, { | |||
import PropTypes from 'prop-types'; | |||
|
|||
import classNames from 'classnames'; | |||
import throttle from 'lodash.throttle'; | |||
import { throttle } from 'es-toolkit/compat'; |
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.
Hey Gui, Thanks for the changes, its Awesome!
I tried to read about throttle from es-toolkit and found that it will accept only 2 parameters like throttle(func, wait)
while for lodash it's like throttle(func, wait, options)
In this file Slider.tsx
we were using throttle from lodash as
onDrag = throttle(this._onDrag, EVENT_THROTTLE, {
leading: true,
trailing: false,
});
Which means the function this._onDrag
is executed immediately when the event is triggered , but it will not be executed again at the end of the throttling interval.
Can you please make sure to change the code as well as per es-toolkit to achieve the same functionality.
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.
Hey Preeti!
I tested with lodash and es-toolkit and the behaviour is the same. We are using the es-toolkit/compat
that has full compatibility with lodash, so we don't have to change the code. All es-toolkit
functions have a "compat" version to ensure we don’t need to change anything in our code.
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.
Oh yes, the link is saying it supports leading
and trailing
options as well. Thanks!
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.
Great! LGTM!
looks like just need to resolve conflicts |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #18162 +/- ##
=======================================
Coverage 84.31% 84.31%
=======================================
Files 404 404
Lines 14352 14353 +1
Branches 4655 4601 -54
=======================================
+ Hits 12101 12102 +1
- Misses 2089 2090 +1
+ Partials 162 161 -1 ☔ View full report in Codecov by Sentry. |
4cf18d3
Closes #17731
We are moving from
lodash
to es-toolkit which is a modern library. That change will solve security issues we were having inlodash
and also it will lead to a smaller final bundle and better performance.Replaced a few lodash code to use pure JavaScript. Here it is a Stackblitz with the code changes examples to show that it works.
Changelog
New
es-toolkit
dependencyChanged
lodash.findlast
to use pure Javascript codelodash.omit
to use pure Javascript codeRemoved
lodash
dependenciesTesting / Reviewing