-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[codemod] Rename shouldDisableTime
prop
#7709
[codemod] Rename shouldDisableTime
prop
#7709
Conversation
These are the results for the performance tests:
|
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.
Looks great! 💯
Just a general question: do you know if such thing is feasible with jscodeshift
:
Having:
const props = {
shouldDisableTime: (timeValue, view) => view === 'hours' && timeValue < 12,
};
<TimePicker {...props} />
transform it into:
const props = {
shouldDisableClock: (timeValue, view) => view === 'hours' && timeValue < 12,
};
<TimePicker {...props} />
Because our current codemods will only cover users directly using pickers from our package as well as specifying props directly on the component, but I fear that in the real world we might have quite a lot of users doing the following:
- having a wrapper component on top of ours with additional logic/defaults
- spreading the props from some other object onto the picker component
docs/data/migration/migration-pickers-v5/migration-pickers-v5.md
Outdated
Show resolved
Hide resolved
packages/x-codemod/src/v6.0.0/pickers/rename-should-disable-time/index.ts
Outdated
Show resolved
Hide resolved
packages/x-codemod/src/v6.0.0/pickers/rename-should-disable-time/actual-props.spec.js
Show resolved
Hide resolved
@LukasTy I don't think it's feasible with For example here is a simple code and it's associated AST in JSON (I removed most of the useless parts) const prop = { label: "Name" }
<A {...props} /> {
"program": {
"type": "Program",
"body": [
{
"type": "VariableDeclaration",
"declarations": [
{
"type": "VariableDeclarator",
"id": {
"type": "Identifier",
"name": "props"
},
"init": {
"type": "ObjectExpression",
"properties": [
{
"type": "Property",
"key": {
"type": "Identifier",
"name": "label"
},
"computed": false,
"value": {
"type": "Literal",
"value": "Nale",
"raw": "'Name'"
},
"kind": "init",
"method": false,
"shorthand": false
}
]
}
}
],
"kind": "const"
},
{
"type": "ExpressionStatement",
"expression": {
"type": "JSXElement",
"openingElement": {
"type": "JSXOpeningElement",
"name": {
"type": "JSXIdentifier",
"name": "A"
},
"selfClosing": true,
"attributes": [
{
"type": "JSXSpreadAttribute",
"argument": {
"type": "Identifier",
"name": "props"
}
}
]
},
"children": [],
"closingElement": null
}
}
]
}
} The tree is made of two parts:
The only way to get the link between them is to notice that the first one defines an object of type As you can imagine, this can not scale since you would have to consider all the possible ways to edit an object. Same if objects are imported (we only parse one file at a time) |
@alexfauquette Thank you for a detailed answer. 🙏 |
Part of #6997