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

Add ability to subscribe to multiple keys and to prevent propagation #49

Merged
merged 19 commits into from
Aug 28, 2020

Conversation

fredyshox
Copy link
Contributor

@fredyshox fredyshox commented Jun 5, 2020

Attempt to fix #30

Added CompositeUserDefaultsKeyObservation for observing multiple keys, with preventPropagation option to prevent infinite recursion when change is made within handler.

I store flag in threadDictionary of the current thread to determine when changes should be propagated. This enables to still support changes made on other threads, while initial one is still processing handler (more in testObservePreventPropagationMultiThread test case).

Code is still a little messy, but let me know what you think :)


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


@fredyshox
Copy link
Contributor Author

As for observeAll and publisherAll for all known keys, Mirror does not yet support static fields, so my initial thought of Mirror(Defaults.Keys.self).children doesn't work.

@sindresorhus
Copy link
Owner

This is looking promising. Thanks for working on it. 🙌

@sindresorhus
Copy link
Owner

As for observeAll and publisherAll for all known keys, Mirror does not yet support static fields, so my initial thought of Mirror(Defaults.Keys.self).children doesn't work.

Too bad. "static" always lags behind in functionality. We could actually solve this without mirror if we switch to using Key paths instead of the dot syntax (It would then be Defaults[\.foo]. But of course "static key paths" are not yet implemented in Swift.

@sindresorhus
Copy link
Owner

Is there any way we could add propagation prevention to publisher(keys:) too?

@fredyshox
Copy link
Contributor Author

Is there any way we could add propagation prevention to publisher(keys:) too?

We could do that, by tweaking DefaultsSubscription into using new CompositeUserDefaultsKeyObservation. But there're few catches. Note that preventPropagation won't protect you, if you use dispatch.async inside observation handler. And in Combine is far more common to use things like receive(on:), debounce, delay etc.

@fredyshox
Copy link
Contributor Author

fredyshox commented Jun 27, 2020

I’ve came up with a different approach, to make this feasible on both classic and combine implementations. A block based approach inspired by SwiftUIs withAnimation:

func withNoPropagation(block: () -> Void) {
	// tweak threadDictionary and call block
}

Every change made in the block, would not be propagated at all. Usage example:

Defaults.observeAll(.key1, .key2) {
	// …
	withNoPropagation {
		// update some value at .key1
		// this will not be propagated
		Defaults[.key1] = 11
	}
	// this will be propagated
	Defaults[.someKey] = true 
}

A big advantage is that, this may be used not only in observation callback, but also in Combine callbacks (regardless of publisher modifiers) or any other place.

I’ve working implementation, let me know what you think 👍

@sindresorhus
Copy link
Owner

@fredyshox That's a good approach! I like it.

Should it be withNoPropagation or withoutPropagation?

@fredyshox fredyshox marked this pull request as ready for review July 6, 2020 14:41
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
Sources/Defaults/Observation.swift Show resolved Hide resolved
Sources/Defaults/Observation.swift Show resolved Hide resolved
Sources/Defaults/Observation.swift Outdated Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Also replied to #49 (comment).

@sindresorhus
Copy link
Owner

Sorry for the late reply. Summer and COVID made it hard to keep up with all the PRs.

@sindresorhus
Copy link
Owner

Can you document withoutPropagation?

@fredyshox
Copy link
Contributor Author

I've added docstrings to new public API and updated readme. Also added short comment explaining inner workings of preventPropagation inside its implementation 🙂.

@sindresorhus sindresorhus changed the title Ability to subscribe to multiple keys or any change Add ability to subscribe to multiple keys and to prevent propagation Aug 28, 2020
@sindresorhus sindresorhus merged commit ab81276 into sindresorhus:master Aug 28, 2020
@sindresorhus
Copy link
Owner

This is looking great. Nice work as always 🙌🏻

@sindresorhus
Copy link
Owner

@fredyshox Would you be interested in joining the project as a maintainer? There's no commitment needed. Just want to highlight your contributions with your name in the readme. No worries if not though.

@fredyshox
Copy link
Contributor Author

Yes I do! Feel free to mention me :)

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.

Add ability to subscribe to multiple keys or any change in a given suite
2 participants