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(translations): memoize array translation #2358

Merged

Conversation

LukasBoll
Copy link
Collaborator

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.

Copy link

netlify bot commented Jul 23, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit 0e697e7
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/66be575ee1c5a000082d87f5
😎 Deploy Preview https://deploy-preview-2358--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sdirix
Copy link
Member

sdirix commented Aug 14, 2024

@LukasBoll Can you fix the build or do you need support?

@LukasBoll LukasBoll force-pushed the resolve_translation_rerender branch 4 times, most recently from 862b6bc to 0e697e7 Compare August 15, 2024 19:30
@coveralls
Copy link

coveralls commented Aug 15, 2024

Coverage Status

coverage: 81.733% (-1.6%) from 83.285%
when pulling c64196b on LukasBoll:resolve_translation_rerender
into 52b843a on eclipsesource:master.

Copy link

netlify bot commented Aug 15, 2024

Deploy Preview for jsonforms-examples ready!

Name Link
🔨 Latest commit c64196b
🔍 Latest deploy log https://app.netlify.com/sites/jsonforms-examples/deploys/66e4636c30eae20008c99350
😎 Deploy Preview https://deploy-preview-2358--jsonforms-examples.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@LukasBoll
Copy link
Collaborator Author

Hi @sdirix,
I just fixed the build issues :)

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.

The PR looks good in general. However there seems to be an issue in Vue Vanilla. When checking the "Nested arrays" example and adding a new element to the outer array, then the nested array within it crashes. Can you check what's going on?

This is the error message:

TypeError: Cannot read properties of undefined (reading 'noDataMessage')
at Proxy.render$l (ArrayListRenderer.vue:44:31)
at renderComponentRoot (runtime-core.esm-bundler.js:887:16)
at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js:6020:46)
at ReactiveEffect.run (reactivity.esm-bundler.js:177:19)
at instance.update (runtime-core.esm-bundler.js:6151:16)
at setupRenderEffect (runtime-core.esm-bundler.js:6161:5)
at mountComponent (runtime-core.esm-bundler.js:5929:7)
at processComponent (runtime-core.esm-bundler.js:5883:9)
at patch (runtime-core.esm-bundler.js:5351:11)
at ReactiveEffect.componentUpdateFn [as fn] (runtime-core.esm-bundler.js:6027:11)

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
@sdirix
Copy link
Member

sdirix commented Aug 21, 2024

@LukasBoll Can you resolve the conflicts?

@sdirix sdirix self-assigned this Sep 11, 2024
@sdirix sdirix added this to the 3.4 milestone Sep 11, 2024
; Conflicts:
;	packages/vanilla-renderers/src/complex/array/ArrayControlRenderer.tsx
@lucas-koehler
Copy link
Contributor

Merge commit only fixes a tiny merge conflict.

@lucas-koehler lucas-koehler merged commit 8cdd5ef into eclipsesource:master Sep 13, 2024
8 checks passed
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.

4 participants