-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Codify lodash usage in eslint #1270
Conversation
Your Render PR Server URL is https://toolpad-pr-1270.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-cdhrruda4994n7g25l8g. |
Signed-off-by: Jan Potoms <2109932+Janpot@users.noreply.github.com>
@@ -34,6 +34,8 @@ import { | |||
JsExpressionAttrValue, | |||
} from '@mui/toolpad-core'; | |||
import { createProvidedContext, useAssertedContext } from '@mui/toolpad-utils/react'; | |||
import { filterKeys, mapProperties, mapValues } from '@mui/toolpad-utils/collections'; | |||
import { set as setObjectPath } from 'lodash-es'; |
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.
If we plan to eventually completely get rid of lodash
we could maybe create utility methods for these operations, which for now would still use lodash
but then we would know where to replace it.
Just a possibility, maybe it helps to have the lodash
imports less spread out.
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.
To be honest, I'm not sure we should even have utility functions for this. ideally we improve the algorithm altogether to be not so lodashy. setObjectPath
also makes us write type-unsafe code, I don't want to enable that sort of code in our codebase
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.
Got it, yeah for this specific type of operation maybe just normal Javascript would be possible and be more type-safe.
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.
Sounds good, should we create an issue to replace lodash in the todo comments?
@@ -1226,7 +1228,7 @@ function QueryNode({ page, node }: QueryNodeProps) { | |||
Object.fromEntries(node.params ?? []), | |||
); | |||
|
|||
const configBindings = _.pick(node.attributes, USE_DATA_QUERY_CONFIG_KEYS); | |||
const configBindings = filterKeys(node.attributes, (key) => USE_DATA_QUERY_CONFIG_KEYS.has(key)); |
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.
We could abstract this to our own pick
function maybe?
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.
Removing it altogether
Avoid kitchensink libraries, especially unmaintained ones like
lodash
. We write a slightly more verbose, but a universally understood style of javascript.