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 popped Record's ProvidedValues lifetime #1086

Merged
merged 4 commits into from
Dec 31, 2023

Conversation

dandc87
Copy link
Contributor

@dandc87 dandc87 commented Dec 30, 2023

Fixes #1065

Copy link

Thanks for the contribution! Before we can merge this, we need @dandc87 to sign the Salesforce Inc. Contributor License Agreement.

Copy link
Contributor

@chrisbanes chrisbanes 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 to me!

We really need to merge all of these 'test' presenters/infra. I think this will make 5 copies 😅. Not a blocker for this PR though. I've got an upcoming change which needs this so will merge them all then.

@chrisbanes
Copy link
Contributor

Just uploaded a PR to merge all of the common test infra: #1088. I'll rebase if this goes in first.

@ZacSweers
Copy link
Collaborator

I'll run the instrumentation tests locally before merging, sorry we have an awkward setup right now for secrets access to run them from external contributors

@ZacSweers
Copy link
Collaborator

actually maybe I can trigger it with an empty commit from my side

@ZacSweers ZacSweers merged commit 4050eab into slackhq:main Dec 31, 2023
1 of 2 checks passed
@ZacSweers
Copy link
Collaborator

Thanks!

@dandc87 dandc87 deleted the koski/1065-repro-test branch December 31, 2023 22:05
@valeriyo
Copy link

valeriyo commented Jan 3, 2024

🎉

github-merge-queue bot pushed a commit that referenced this pull request Jan 5, 2024
Fixes original issue reported in
#1065

The fix in #1086 only made sure
the correct ViewModelStore was present, but `ViewModel.onCleared()` is
still being called upon stack-pop, not once the Presenter/Ui completely
left composition.

This PR tracks both "stack" lifetime and "ui" lifetime and only clears
the ViewModelStore when both have been forgotten. I piggybacked on
shared `RememberType` stuff, I hope that's alright.

Also fixes `ProvidedValuesLifetimeTest` to read the CompositionLocal
again, using better special values to hopefully prevent accidental
breaking again
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.

ViewModel onCleared called too early
4 participants