refactor: change defaultProps generation for react #1661
Conversation
🦋 Changeset detectedLatest commit: d3346e5 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 d3346e5.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
@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. ^.^ |
…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
| "'function' is not allowed as a variable declaration name. (19:7) | ||
| 17 | | ||
| 18 | const name= 'Steve' | ||
| > 19 | const function underscore_fn_name() { |
There was a problem hiding this comment.
interesting...looks like your function refactor impacts stateType: 'variables' config on react.
It fixes a handful of cases where we used to have
// before - invalid
const onChange = onChange = function function onChange() {
// after - valid
const onChange = function onChange() {but it also breaks the following:
//before - valid
const underscore_fn_name = function underscore_fn_name() {
//after - invalid
const function underscore_fn_name() {Would this be fixed if your rscPrefix workaround applied all the time? If you don't mind testing this out, could be a huge benefit to the generator.
If it ends up causing more headaches, we can ignore and merge without the fix.
There was a problem hiding this comment.
I found the issue (I guess).
The original issue had happend only for functions outside of the state/store
…ault-props # Conflicts: # e2e/e2e-react/src/App.tsx # packages/core/src/__tests__/__snapshots__/stencil.test.ts.snap # packages/core/src/generators/stencil/component.ts # packages/core/src/generators/stencil/helpers/index.ts
…to refactor-react-default-props
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:updateyarn g:changesetand follow the CLI instructions. Alternatively, use the Changeset Github Bot to create the file.