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 component cache for polymorphic field re-renders #2189

Merged
merged 11 commits into from
Mar 6, 2025

Conversation

tintinthong
Copy link
Contributor

@tintinthong tintinthong commented Feb 24, 2025

The point of this PR is to enable polymorphic fields to re-render upon assigning a subclass to a field of baseclass type. Previously, although the JSON is correctly serialised, the templates were not re-rendering

Screenshot 2025-02-24 at 16 03 18 Screenshot 2025-02-24 at 16 03 30

Solution

The main issue is that getBoxComponent was serving a stale cache (componentCache) bcos it did not take into account changes in the card or field class ie cardOrField. So we added a value inside the cache storing cardOrField which then serves the most up-to-date BoxComponent if cardOrField changed.

Digression Risk

As part of pairing on Thursday with @ef4, the outer cache at fieldsComponentsFor (stableComponents) was already serving a stale cache that was preventing the code from returning the most up-to-date cache in boxComponent. This several layers of caching made it difficult to debug. We initially thought that this outer-cache was not needed.

BUT afterwards I found out that the cache was made for the purpose preventing eager re-renders that caused linksToManyComponent and containsManyComponent to re-render -- so I kept the cache in place. Here is the relevant PR #860 by @jurgenwerk. I eventually, decided to keep the cache for specific fields ie plural fields like linksToManyComponent and containsManyComponent for simplicity.. also to unblock Spec and Playground work.

I plan to pair further with @ef4 to find the best solution here.. I think the more general solution wud be to allow getBoxComponent to have better handling for plurals.

Copy link

github-actions bot commented Feb 24, 2025

Host Test Results

    1 files  ±0      1 suites  ±0   24m 19s ⏱️ +19s
792 tests +3  790 ✔️ +3  2 💤 ±0  0 ±0 
797 runs  +3  795 ✔️ +3  2 💤 ±0  0 ±0 

Results for commit a894b3d. ± Comparison against base commit 17d0250.

♻️ This comment has been updated with latest results.

@tintinthong tintinthong changed the title Fix component cache for field Fix component cache for polymorphic field re-renders Feb 24, 2025
@tintinthong tintinthong force-pushed the fix-component-cache-for-field branch from 5acf4bb to 5e6b566 Compare February 24, 2025 07:54
@tintinthong tintinthong requested review from ef4, jurgenwerk, a team and lukemelia February 24, 2025 08:22
@burieberry
Copy link
Contributor

Is the template not rerendering when clicking the button still an issue? I tried on latest main and didn't have any issues.

@tintinthong
Copy link
Contributor Author

tintinthong commented Feb 26, 2025

Is the template not rerendering when clicking the button still an issue? I tried on latest main and didn't have any issues.

wdym. Where did u see that there was a re-render of the field? Particularly, when ur somePersonField=contains(FieldDef)

@tintinthong
Copy link
Contributor Author

Is the template not rerendering when clicking the button still an issue? I tried on latest main and didn't have any issues.

I tried this test on main. I dont think it works

Screenshot 2025-02-27 at 02 46 23

@burieberry
Copy link
Contributor

Is the template not rerendering when clicking the button still an issue? I tried on latest main and didn't have any issues.

I tried this test on main. I dont think it works

Sorry ok I was able to reproduce the issue. Previously didn't see it because I was switching between formats on code mode preview. Confirm that this PR resolves it.

@tintinthong tintinthong force-pushed the fix-component-cache-for-field branch from 74384e1 to 332df0d Compare March 3, 2025 12:15
@tintinthong
Copy link
Contributor Author

tintinthong commented Mar 3, 2025

I think this is ready for review

The continuation of this PR. I have removed the fields proxy cache and moved the cache into the linksToManyComponent. containsMany and linksToMany examples should still have the stability they have had previously.

@tintinthong
Copy link
Contributor Author

tintinthong commented Mar 5, 2025

Its great that the change in cache introduced inside getBoxComponent (that makes cardOrField as a dependency) here fixes the re-ordering template issue seen here #2241.

Essentially, it invalidates the componentCache of the box component but that also means any component state is destroyed (without auto-save).

Copy link
Contributor

@ef4 ef4 left a comment

Choose a reason for hiding this comment

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

We think this is a reasonable step forward without taking on a big refactor.

It's hard to be sure this doesn't change component stability somewhere else, but we haven't been able to find any examples in the tests. So we can move ahead with this and folks should be on the lookout for any component state loss regressions that we can debug.

@tintinthong tintinthong merged commit 4dc9310 into main Mar 6, 2025
48 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.

3 participants