-
Notifications
You must be signed in to change notification settings - Fork 583
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
refactor: change defaultProps generation for react #1661
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d1bd12e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
View your CI Pipeline Execution ↗ for commit d1bd12e.
☁️ Nx Cloud last updated this comment at |
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.
@nmerget Thanks a lot for tackling this long overdue refactor!
There is however one big drawback to your approach: by writing props = { /* DEFAULT_PROPS */}
, you are overwriting any props that do not have defaults!
See this example:
type Props = {
foo?: string;
bar?: string;
};
// BEFORE
const A = (props: Props) => <div>B: {JSON.stringify(props)}</div>;
A.defaultProps = { foo: 'bar' };
// AFTER
const B = (props: Props = { foo: 'bar' }) => (
<div>A: {JSON.stringify(props)}</div>
);
// FIX
const C = (props: Props) => {
props = { ...props, foo: 'bar' };
return <div>C: {JSON.stringify(props)}</div>;
};
export default function App() {
return (
<>
<A bar="another-prop" />
<B bar="another-prop" />
<C bar="another-prop" />
</>
);
}
I think we need to instead use the approach i provide in C
This would always overwrite the given props with the default value, wouldn't it? :P |
Woops, you're right. I should have written: const C = (props: Props) => {
props = { foo: 'bar', ...props };
return <div>C: {JSON.stringify(props)}</div>;
}; To re-illustrate my point, here's an updated example: As you can see, Essentially, |
Yes, I changed it to follow your suggestions. I also had to change other targets because the e2e tests were failing. ^.^ |
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.
Requesting changes because one of your refactors caused a regression in the react generator
if (type === 'getter') return getFunctionString(code.replace('get ', '')); | ||
if (type === 'function') | ||
return code.startsWith('async') ? code : getFunctionString(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.
this refactor seems to have introduced the regression: getFunctionString(code)
doesn't do the same thing as ${key} = function ${code}
in the original 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.
I'll look at 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.
Quick fix was to have an exception for rsc
^.^
…ault-props # Conflicts: # packages/core/src/__tests__/__snapshots__/react.test.ts.snap # packages/core/src/__tests__/__snapshots__/stencil.test.ts.snap # packages/core/src/generators/stencil/component.ts
Description
Please provide the following information:
closes #1471
Make sure to follow the PR preparation steps in CONTRIBUTING.md before submitting your PR:
yarn fmt:prettier
.yarn test:update
yarn g:changeset
and follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.