-
Notifications
You must be signed in to change notification settings - Fork 29
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
New codemod to help migrating React components "defaultProps" usage #3681
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 4df345c The changes in this PR will be included in the next version bump. This PR includes changesets to release 36 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
I can follow along in a reasonable level of detail.
return <div>{prop1}</div>; | ||
} | ||
``` | ||
*/ |
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.
Thanks for the comments!
...defaultPropsKeys.map((key) => { | ||
const id = j.identifier(key); | ||
const newProp = j.property('init', id, id); | ||
newProp.shorthand = true; |
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.
Wasn't aware of this.
// Default props can be defined inline or as a reference to another object | ||
// INLINE -- MyComponent.defaultProps: { prop1: 'value1', prop2: 'value2' } | ||
// REFERENCE -- MyComponent.defaultProps: defaultProps | ||
if (defaultPropsNode.type === 'Identifier') { |
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.
Good point. Didn't think of this use case.
); | ||
} | ||
} | ||
} |
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.
I can follow along. Took me a while to realize - thanks to the comments - that we need to handle different ways in which React.FCs can be written.
Co-authored-by: Tobias Deekens <[email protected]>
Co-authored-by: Tobias Deekens <[email protected]>
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.
Thank you, especially for the step by step comments 🙌🏽
member.key.type === 'Identifier' && | ||
destructuredKeys.includes(member.key.name) | ||
) { | ||
member.optional = true; |
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.
This is where we make the type member of the prop optional when it is part of the prop right ?
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.
Yes. I need to add another comment.
updateComponentTypes({ | ||
j, | ||
root, | ||
typeName: `${componentName}Props`, |
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.
This is also something I noticed, if we build the typeName from this, sometimes, there are cases where the typeName is inconsistent and we end up having files with typeNames as just ${componentName}
without the Props suffix.
This also would have to be updated manually for cases like these.
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.
Yes, this relies on us using a consistent naming.
I think I can improve it further as a follow-up.
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 can remove the suffix Props
from the existing name to make sure we can append it. If the component name doesn't end up with Props
, the replace
doesn't do anything and we can still append the suffix correctly.
typeName: `${componentName}Props`, | |
typeName: `${componentName.replace(/(Props)$/, '')}Props`, |
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 run this codemod on the 100+ component in the ui-kit
repository and didn't find this problem so I would prefer to actually have a use case for changing it.
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.
I've just realized I was wrong. I had to adjust some things manually in ui-kit
but didn't think it was because of this.
However, I would like to do it as a follow-up in order to not expand the scope of this PR.
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.
I would like to do it as a follow-up in order to not expand the scope of this PR
Do you mean adding .replace(/(Props)$/, '')
? Or are you referring to other things?
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.
I had a more elaborated idea in mind but I already implemented while I was waiting for the review.
Please take a look at this commit: 9af02f7
refactoredParameter.typeAnnotation = functionPropsParam.typeAnnotation; | ||
break; | ||
default: | ||
console.warn( |
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.
🙌🏽
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.
Thanks @CarlosCortizasCT ! 🙌
Could you write some tests for this new codemod? See test folder.
@emmenko that's a legit request but the problem is that If you have any suggestion on how to easily implement the tests, please let me know. |
Ah ok, sorry I didn't know that it's an issue in the test if we use Then it's fine as long as you are confident that it works as expected 🤗 |
So far I'm doing manual testing to verify everything works as expected so I'm pretty confident. |
…com/commercetools/merchant-center-application-kit into fec-138-react-default-props-codemod
…com/commercetools/merchant-center-application-kit into fec-138-react-default-props-codemod
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.
💯
Summary
The way we are using React components
defaultProps
in our codebase is no longer supported in the new React 19 version (reference).This is the main concept:
Description
There are multiple adjustments that are needed and this codemod does not address 100% of them but it helps with most of the common use cases. However, a double check review is recommended after running it; at least checking the Typescript types and building the codebase.
These are the main steps the codemod goes through:
defaultProps
propertydefaultProps
assignment from the componentNOTE
I tried to be very verbose with the inline comments of the codemod explaining what every block is for.