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

Unexpected oneOf re-rendering when filling other schema fields [React Material] #2342

Open
andrecchia opened this issue May 30, 2024 · 4 comments

Comments

@andrecchia
Copy link

Describe the bug

Starting from version 3.2.0, with React Material renderers, I observed that any keyboard input in any field triggers a rerender of the oneOf elements.

Expected behavior

The oneOf elements should rerender only when there is keyboard input within their own fields, as was happening in version 3.1.0

Steps to reproduce the issue

  • Clone the react seed
  • Add this src/CustomText renderer that wraps the MaterialTextControl inserting a Math.random() to make the rerendering evident
      import {
          ControlProps,
          isStringControl,
          RankedTester,
          rankWith
        } from '@jsonforms/core';
        import { withJsonFormsControlProps } from '@jsonforms/react';
        import { Unwrapped } from '@jsonforms/material-renderers';
        import React from 'react';
        const { MaterialTextControl } = Unwrapped;
        
        export const CustomText = React.memo((props: ControlProps) => (
          <>
          <MaterialTextControl {...props} />
          {Math.random()}
          </>
        ));
        export const customTextTester: RankedTester = rankWith(
          1+100,
          isStringControl
        );
        export default withJsonFormsControlProps(CustomText);
  • replace the src/schema.json with this, containing a text field and a oneOf:
    {
      "definitions": {
        "address": {
          "type": "object",
          "title": "Address",
          "properties": {
            "street_address": {
              "type": "string"
            },
            "city": {
              "type": "string"
            },
            "state": {
              "type": "string"
            }
          },
          "required": [
            "street_address",
            "city",
            "state"
          ]
        },
        "user": {
          "type": "object",
          "title": "User",
          "properties": {
            "name": {
              "type": "string"
            },
            "mail": {
              "type": "string"
            }
          },
          "required": [
            "name",
            "mail"
          ]
        }
      },
      "type": "object",
      "properties": {
        "testText":{
          "type": "string"
        },
        "addressOrUser": {
          "oneOf": [
            {
              "$ref": "#/definitions/address"
            },
            {
              "$ref": "#/definitions/user"
            }
          ]
        }
      }
    }
  • Add the CustomText renderer { tester: customTextTester, renderer: CustomText } to src/App.tsx's array of renderers, comment the uischema in JsonForms element (the default uischema is enough)
  • Start the application, fill the Test Text field and observe that everything is rerendering, oneOf included, even if there should be no reason for the oneOf to rerender
  • Now, switch to the JSONForms version v3.1.0: git checkout 3ed669c5e6434919b090c7c9981f3c9a643c125c, and reinstall the dependencies npm install; repeat the steps at the previous point and observe that when filling Test Text there is no oneOf rerendering, as it should be.

Screenshots

No response

Which Version of JSON Forms are you using?

v3.3.0

Framework

React

RendererSet

Material

Additional context

No response

@sdirix
Copy link
Member

sdirix commented Jun 4, 2024

Hi @andrecchia,

Thanks for the detailed report! Much appreciated!

It seems we accidentally broke the memo barrier somewhere in between the versions. Did you already check via the React Dev Tooling which prop is responsible for the rerendering of our components?

@sdirix sdirix added the bug label Jun 4, 2024
@sdirix sdirix added this to the 3.x milestone Jun 4, 2024
@andrecchia
Copy link
Author

Hi @sdirix, sorry for the very late reply.
Actually I tried but I could not get the responsible of the rerendering, I am not so used to React Dev Tooling.
I plan to check for it again since in these days I am debugging my application, I will get you updated.

@LukasBoll
Copy link
Contributor

This error occurs in the OnOf-renderer and the Array-renderer. They use a set of translated texts (e.g. for the confirmation dialog). But we create a new translations object each time when mapping the JSONforms state to component props in the core module:

export const getCombinatorTranslations = (
t: Translator,
defaultTranslations: CombinatorDefaultTranslation[],
i18nKeyPrefix: string,
label: string
): CombinatorTranslations => {
const translations: CombinatorTranslations = {};
defaultTranslations.forEach((controlElement) => {
const key = addI18nKeyToPrefix(i18nKeyPrefix, controlElement.key);
translations[controlElement.key] = t(key, controlElement.default(label));
});
return translations;

export const getArrayTranslations = (
t: Translator,
defaultTranslations: ArrayDefaultTranslation[],
i18nKeyPrefix: string,
label: string
): ArrayTranslations => {
const translations: ArrayTranslations = {};
defaultTranslations.forEach((controlElement) => {
const key = addI18nKeyToPrefix(i18nKeyPrefix, controlElement.key);
translations[controlElement.key] = t(key, controlElement.default(label));
});
return translations;
};

@sdirix
Copy link
Member

sdirix commented Jul 2, 2024

Good find. This means we need to handle them differently in the bindings to avoid breaking the memo barrier. Either by customizing the memo handling or by binding them separately (in a transparent fashion).

@lucas-koehler lucas-koehler modified the milestones: 3.x, 4.x Jul 5, 2024
LukasBoll added a commit to LukasBoll/jsonforms that referenced this issue Aug 15, 2024
This commit addresses an issue where array translations were created as new objects within the core module for each render
cycle, causing unnecessary rerenders. By memoizing the translation object in the material renderer set, this commit optimizes
performance and prevents redundant rerenders.

closes eclipsesource#2342
LukasBoll added a commit to LukasBoll/jsonforms that referenced this issue Aug 15, 2024
This commit addresses an issue where array translations were created as new objects within the core module for each render
cycle, causing unnecessary rerenders. By memoizing the translation object in the material renderer set, this commit optimizes
performance and prevents redundant rerenders.

closes eclipsesource#2342
LukasBoll added a commit to LukasBoll/jsonforms that referenced this issue Aug 15, 2024
This commit addresses an issue where array translations were created as new objects within the core module for each render
cycle, causing unnecessary rerenders. By memoizing the translation object in the material renderer set, this commit optimizes
performance and prevents redundant rerenders.

closes eclipsesource#2342
LukasBoll added a commit to LukasBoll/jsonforms that referenced this issue Aug 15, 2024
This commit addresses an issue where array translations were created as new objects within the core module for each render
cycle, causing unnecessary rerenders. By memoizing the translation object in the material renderer set, this commit prevents redundant rerenders.
This commit also adds memorization to the attributes of the material oneOfRenderer to further avoid unnecessary rerendering.

closes eclipsesource#2342
LukasBoll added a commit to LukasBoll/jsonforms that referenced this issue Aug 18, 2024
This commit addresses an issue where array translations were created as new objects within the core module for each render
cycle, causing unnecessary rerenders. By memoizing the translation object in the material renderer set, this commit prevents redundant rerenders.
This commit also adds memorization to the attributes of the material oneOfRenderer to further avoid unnecessary rerendering.

closes eclipsesource#2342
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants