Skip to content
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

Change recompose methods usage for React.memo #4786

Merged
merged 9 commits into from
May 27, 2020

Conversation

WiXSL
Copy link
Contributor

@WiXSL WiXSL commented May 7, 2020

@WiXSL
Copy link
Contributor Author

WiXSL commented May 10, 2020

Note:
The Enhanced... version of the returning components pass integration tests. returning memo(Component) directly did not.
If this is ok, ArrayField component has to be changed like that as well.

Let me know that or if you find a cleaner solution.

@fzaninotto
Copy link
Member

I think the problem comes from the fact that SimpleForm inspect its children props looking for an addLabel value. React-admin field props set this as defaultProps. But if you decorate with React.memo after setting the defaultProps, this no longer works.

Therefore, I think you should set the React.memo call earlier in all fields:

-const TextField = props = > ( ...);
+const TextField = memo<FieldProps & TypographyProps>(props = > ( ...));
-const EnhancedTextField = memo<FieldProps & TypographyProps>(TextField);
-EnhancedTextField.defaultProps = {
-    addLabel: true,
-};
+TextField.defaultProps = {
+    addLabel: true,
+};
-export default EnhancedTextField;
+export default TextField;

Also, some of the tests import the non-decorated Field (import { TextField } from 'TextField' instead of import TextField from 'TextField'). This shouls no longer be necessary when using React.memo.

@fzaninotto fzaninotto merged commit 2883770 into marmelab:next May 27, 2020
@fzaninotto
Copy link
Member

Thanks!

@fzaninotto fzaninotto added this to the 3.6.0 milestone May 27, 2020
@WiXSL WiXSL deleted the ra-recompose-to-memo branch May 27, 2020 14:28
@Luwangel Luwangel mentioned this pull request Jul 27, 2020
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants