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

Cache modifies view instead of setting #4055

Merged
merged 2 commits into from
Aug 13, 2024
Merged

Cache modifies view instead of setting #4055

merged 2 commits into from
Aug 13, 2024

Conversation

toddburnside
Copy link
Contributor

@toddburnside toddburnside commented Aug 13, 2024

Instead of having the cache maintain the state and using it as the final source of truth, the cache now uses the mod function of a view of the state to update the state when changes arrive. This resolves some UI issues. For example:

  • If changes were made to the UI while the delayed inits for the ProgramSummaries were coming in, the summaries would be updated as each delayed init value came in. This resulted in the change disappearing from the UI and then reappearing later when the subscription event from the server for that change was received and processed.
  • For adding a brand new target to an asterism (with upcoming undo/redo changes in place), the Action would update the UI to the new target in the asterism. However, when the targetEdit event for adding the new target came in, it would overwrite the summaries and make that UI state invalid. The subsequent *Edit events would put the cache in the state where the UI WOULD have been valid, but the UI had already been updated.

Copy link

bundlemon bot commented Aug 13, 2024

BundleMon

Files updated (1)
Status Path Size Limits
index-(hash).js
1.66MB (+932B +0.05%) -
Unchanged files (7)
Status Path Size Limits
exploreworkers-(hash).js
600.27KB -
index-(hash).css
65.51KB -
workbox-window.prod.es5-(hash).js
2.07KB -
catalogworker-(hash).js
91B -
plotworker-(hash).js
88B -
agsworker-(hash).js
87B -
itcworker-(hash).js
74B -

Total files change +932B +0.04%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

Copy link
Contributor

@rpiaggio rpiaggio left a comment

Choose a reason for hiding this comment

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

LGTM, thank you for catching this and implementing this fix.

I'm not sure how we got to the previous version, but it seems at some point I wanted to keep the cache value in the SignallingRef, but ended up having it in the RootModel, thus resulting in a weird hybrid.

What you made now looks much more like what we want.

Since CacheComponent and its subtypes are not really caches (the state is external to them), could we take the opportunity to rename it to CacheControllerComponent (or similar) and the subtypes to ProgramCacheController, etc. ?

@toddburnside
Copy link
Contributor Author

LGTM, thank you for catching this and implementing this fix.

De nada. The framework you put in place made it pretty easy.

Since CacheComponent and its subtypes are not really caches (the state is external to them), could we take the opportunity to rename it to CacheControllerComponent (or similar) and the subtypes to ProgramCacheController, etc. ?

Sure. I'll do that.

@toddburnside toddburnside marked this pull request as ready for review August 13, 2024 16:47
@toddburnside toddburnside merged commit d881530 into master Aug 13, 2024
14 checks passed
@toddburnside toddburnside deleted the cache branch August 13, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants