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

Redesign @AsyncOpen and @AutoOpen initialisers to allow setting a client reset modes for any sync configuration #8109

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dianaafanador3
Copy link
Contributor

Redesign @AsyncOpen and @AutoOpen initialisers to allow setting a client reset mode for any sync configuration, and avoid confusion with the configuration and the sync configuration from the user on each intialiser.

Currently, setting a client reset mode for @AsyncOpen and @AutOOpeN is not working correctly, which mainly has to do with the implementation approach within the property wrappers which uses the sync configuration from the sync type (depending on the initialiser, flx or pbs) and combine it with the injected configuration (which can come from the initialiser or and environment value).
In this PR I'm proposing a redesign of the initials for the both @AsyncOpen and @AutOOpeN to avoid confusion of which sync configuration is been used internally.

This PR has some breaking changes, which only should affect users in cases where they were using this feature incorrectly (for example, if they were injecting a configuration with a different partition value that the one on the initialiser or a configuration with a different sync type that the one from the initialiser).

@cla-bot cla-bot bot added the cla: yes label Jan 23, 2023
@dianaafanador3 dianaafanador3 force-pushed the dp/redesign_asyncopen_init branch 2 times, most recently from 2132964 to 0824d78 Compare January 23, 2023 11:05
CHANGELOG.md Outdated
Comment on lines 12 to 18
* `AsyncOpen.init(appId:partitionValue:configuration:timeout)` and
`AutoOpen.init(appId:partitionValue:configuration:timeout)` will change behavior. Previously
injecting both the configuration and a partition value will make the property wrappers to set
the injected configuration (if not nil) combine with the syncConfiguration from the
`user.configuration(partitionValue:)`. This will change to either user the injected configuration (if not nil)
or the configuration obtained from `user.configuration(partitionValue:)` for the current logged user.
This API will be deprecated on the next major release.
Copy link
Member

Choose a reason for hiding this comment

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

Both the old and new behavior seems incorrect? If a non-nil configuration is provided I would expect it to use exactly that configuration except with the partition value changed to the supplied one.

timeout: UInt? = nil) {
let app = ObservableAsyncOpenStorage.configureApp(appId: appId, timeout: timeout)
// Store property wrapper values on the storage
storage = ObservableAsyncOpenStorage(asyncOpenKind: .asyncOpen, app: app, configuration: nil, partitionValue: nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the configuration is nil how does it know about the initialSubscriptions?

@tgoyne
Copy link
Member

tgoyne commented Feb 10, 2023

This is an unnecessary breaking change which breaks things that seem completely reasonable. @AutoOpen(partitionValue: "foo", configuration: .init(objectTypes: [Bar.self])) is different from how configurations are defined elsewhere, but it's not an invalid thing to do.

… client reset mode for any sync configuration,

and avoid confusion with the configuration and the sync configuration from the user on each intialiser.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants