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

Fix/remove are equal #1814

Merged
merged 2 commits into from
Oct 14, 2021
Merged

Conversation

eneufeld
Copy link
Member

@eneufeld eneufeld commented Oct 7, 2021

No description provided.

@eneufeld eneufeld requested a review from sdirix October 7, 2021 07:52
@coveralls
Copy link

coveralls commented Oct 7, 2021

Coverage Status

Coverage decreased (-0.06%) to 88.98% when pulling eb774aa on eneufeld:fix/removeAreEqual into 3f706fe on eclipsesource:master.

Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it very much and could barely find any problems.

The ExpandPanelRenderershould also be placed in a React.memo and it's methods moveUp etc. should be placed in useCallbacks. At the moment an ExpandPanel rerenders even when only a control in a sibling changes, this can be checked via the "array with detail" example.

Also other renderers are missing their memo. For example the DateControl. Please check that all controls now have a memo.

Please also add to the migration guide for JSON Forms 3.0 that custom renderers now need to be memoized by themselves as it's no longer automatically included in the bindings, e.g. withJsonFormsControlProps. Show an example of how it now should look like.

packages/react/src/JsonFormsContext.tsx Outdated Show resolved Hide resolved
@eneufeld eneufeld force-pushed the fix/removeAreEqual branch 4 times, most recently from dacc020 to 51732e2 Compare October 12, 2021 10:06
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

Found a bug, a missing debounce and some unnecessary useMemo and useCallbacks.


I went with a profiler over the examples. I saw that in nested arrays unrelated expand panels are rerendered even when nothing in their content changes (not at all of them as at some point the content renders a memoized renderer again). Maybe there is some optimization potential.

Some enum table cells also seem to render unnecessarily often, seemingly related to the options object.

I'm fine extracting these issues into a follow up if there is not enough time for them in this task.

MIGRATION.md Outdated Show resolved Hide resolved
packages/react/src/JsonFormsContext.tsx Outdated Show resolved Hide resolved
packages/react/src/JsonFormsContext.tsx Outdated Show resolved Hide resolved
packages/material/src/controls/MaterialNativeControl.tsx Outdated Show resolved Hide resolved
packages/material/src/controls/MaterialRadioGroup.tsx Outdated Show resolved Hide resolved
packages/material/src/controls/MaterialSliderControl.tsx Outdated Show resolved Hide resolved
packages/react/src/DispatchCell.tsx Outdated Show resolved Hide resolved
@sdirix sdirix force-pushed the fix/removeAreEqual branch 2 times, most recently from 74c0a67 to 47f5a74 Compare October 14, 2021 11:37
Due to previous refactorings data instance references only
change on update when their actual content changed. Therefore
the manual memoization comparison functions ("areEqual")
involving deep equals are no longer necessary.

Other props which changed with each rerendering, like the
enum "options" objects or the array dispatch methods are now
also properly memoized.

Includes minor additional changes like refactoring all
remaining React Material renderers to functional compoonents,
generalizing the React bindings and memoizing intermediate
components.
The React Vanilla renderers add additional bindings for style
customizations. The calculated props are now properly memoized
so that the standard React memoization can take effect.
@sdirix sdirix merged commit 81e4dd6 into eclipsesource:master Oct 14, 2021
@sdirix sdirix mentioned this pull request Nov 10, 2021
@sdirix sdirix linked an issue Nov 10, 2021 that may be closed by this pull request
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.

Rework React memo
3 participants