Remove misleading assertion in Spice type#25
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This removes the assert in
Spicethat was warning developers to callprepareIfNeeded()too early. That assertion didn’t accurately reflect how nested spice stores initialize and led to incorrect user defaults key paths.Description
Setting up an observer in the initializer of a nested
SpiceStorewill trigger the assert before the parent–child relationship is wired up. Developers will work around this by callingprepareIfNeeded()too early, which results in an incorrect key path for the child spice.Motivation and Context
Consider the following scenario.
Accessing
$appServicehere triggers our assertion. That happens because initializingEnvironmentSpiceStoreinstalls the Combine observer onappServicebefore the spice store's parent–child relationship is set up. Developers then try to work around the assert by callingprepareIfNeeded()in the child's initializer. This causes the child spice to use the wrong user defaults key, since its fully qualified key path isn’t known yet, ultimately making it appear that the value isn't properly saved because it's saved under an incorrect key.Removing the assertion makes this pattern valid. The nested store will be prepared automatically when its parent is ready.
If we ever find ourselves needing this assert again, we should likely address it differently.
Screenshots (if appropriate):
Types of changes