-
Notifications
You must be signed in to change notification settings - Fork 293
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
feat: add React 19 replace default props transform #331
base: master
Are you sure you want to change the base?
feat: add React 19 replace default props transform #331
Conversation
thanks @amirabbas-gh (and @mohebifar)! @rickhanlonii, this is the first of many PRs from Codemod. appreciate your review! |
23b6272
to
dc9175f
Compare
dc9175f
to
0d7dc9f
Compare
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.
How does this handle more complex types like objects? One issue with migrating from .defaultProps
to function default parameters is that creating an object in the default parameter would invalidate memoization too often.
E.g.
function Component({ options = {} }) {}
is not equivalent to
function Component({ options }) {}
Component.defaultProps = { options: {} }
you'd have to to hoist those values out e.g.
const defaultOptions = {}
function Component({ options = defaultOptions }) {}
const C = (props) => { | ||
return <>{props.text}</>; | ||
}; |
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 no longer sets props.text
by default. The codemod should opt out here with a message (or insert a comment) that it couldn't handle this 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.
@amirabbas-gh Can we do something like this?
Before:
const C = (props) => {
const someValue = props.text;
const { text } = props;
return <>{props.text}</>;
};
After:
const C = (props) => {
const props_textWithDefault = typeof props.text === 'undefined' ? 'test' : props.text;
const someValue = props_textWithDefault;
const { text = props_textWithDefault } = props;
return <>{props_textWithDefault}</>;
};
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.
It won't be able to handle the case of computed member expressions, which is ok, and perhaps we should just leave a note/comment for such cases:
const C = (props) => {
const key = cond ? "text" : "title";
return <>
{/* Codemod: Please make sure this is not affected by removal of defaultProps */}
{props[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.
@eps1lon
I have another proposal:
const C = (props) => { | |
return <>{props.text}</>; | |
}; | |
const C = (props) => { | |
props = {...props, text: typeof props.text === 'undefined' ? 'default' : props.text}; | |
return <>{props.text}</>; | |
}; |
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 seems fine.
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.
@eps1lon
I fixed this in:
this commit
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.
TypeScript is probably going to be the biggest blocker here. But codemodding that perfectly seems like a pretty huge task.
@@ -0,0 +1,12 @@ | |||
const Link = ({ href, children, ...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.
What would happen if href
or children
are not named here e.g. { href, ...props}
and <a href={href} {...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.
For the example you provided, it only considers href and produces the following output:
const Link = ({ href = "#", ...props }) => {
return (
<a href={href} {...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.
@eps1lon
We reviewed this case with @mohebifar 's help, and I understood your point. What do you think about doing something similar to the solution I suggested for the previous case?
Before:
const MyComp = ({foo, ...props}) {
console.log(props.bar)
}
MyComp.defaultProps = {foo: "hello", bar: "bye"}
After:
const MyComp = ({foo = 'hello', ...props}) {
props = {...props, bar: typeof props.bar === 'undefined' ? 'bye' : props.bar}
console.log(props.bar)
}
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.
@eps1lon
I fix it in this commit:
13e3d842e861a8f68c99a07b034dbd83a40cc96c
Hoisted default object values outside function parameters to avoid invalidating memoization when migrating from .defaultProps to function default parameters.
@eps1lon |
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 good. I'd recommend running this on some actual codebases to verify. I don't think we have any more defaultProps
but I can test it on our larger codebases internally as a sanity check. Can you ping me once npx codemod react/19/replace-default-props
is available?
Final version published on Codemod registry! 🚀The final version has been published! All updates are now available on the Codemod registry. You can now run: npx codemod react/19/replace-default-props to execute the codemod with the latest improvements. @eps1lon Let me know if you need anything! 😊 |
great job @amirabbas-gh and thank you @eps1lon for code review. i also tested the codemod and all lgtm. this can be merged now... cc @rickhanlonii , @mohebifar |
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't run the tests locally.
$ yarn test default-props
● Validation Warning:
Unknown option "extensionsToTreatAsEsm" with value [".ts", ".tsx", ".js", ".jsx"] was found.
This is probably a typing mistake. Fixing it will remove this message.
Configuration Documentation:
https://jestjs.io/docs/configuration.html
ts-jest[versions] (WARN) Version 4.8.4 of typescript installed has not been tested with ts-jest. If you're experiencing issues, consider using a supported version (>=2.7.0 <4.0.0). Please do not report issues in ts-jest if you are using unsupported versions.
ts-jest[config] (WARN) TypeScript diagnostics (customize using `[jest-config].globals.ts-jest.diagnostics` option):
message TS151001: If you have issues related to imports, you should consider setting `esModuleInterop` to `true` in your TypeScript configuration file (usually `tsconfig.json`). See https://blogs.msdn.microsoft.com/typescript/2018/01/31/announcing-typescript-2-7/#easier-ecmascript-module-interoperability for more information.
FAIL transforms/__tests__/react-19-replace-default-props.codemod.test.ts
● Test suite failed to run
Cannot find module '@codemod.com/codemod-utils' from 'react-19-replace-default-props.codemod.ts'
The warnings also seem concerning. Especially related to ts-jest
when we just added this dependency.
@eps1lon There were some issues in the PR as well as in some of Codemod's packages, which I have fixed. Please pull changes, reinstall the packages, and make sure to run the tests again. |
📚 Description
This PR is created to update the react-codemod repository with the updates for the react/19/replace-default-props codemod and to align the open-source content of this codemod in react-codemod similar to the Codemod repository.
Added Codemod Utils Package
@codemod.com/codemod-utils
package as a dependency to facilitate codemod development.Added Custom TypeScript Configuration for Codemods
react-19-replace-default-props
to handle default props migration for React 19.npx codemod react/19/replace-default-props
🧪 Test Plan
The following steps were taken to ensure the changes function as intended:
react-19-replace-default-props
transform was executed against test cases.