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

Rename ImmutableObservable to Observable #24

Closed
rharter opened this issue Mar 31, 2019 · 4 comments
Closed

Rename ImmutableObservable to Observable #24

rharter opened this issue Mar 31, 2019 · 4 comments

Comments

@rharter
Copy link
Contributor

rharter commented Mar 31, 2019

The ImmutableObservable class is great, since it allows you to have a public API in your classes that is safe from external side effects, but the naming semantics are backwards.

The current name would imply that ImmutableObservable is a special kind of Observable that adds immutability, but that's not the case. Instead Observable is a special kind of ImmutableObservable that adds mutability, but that's not apparent from reading the names.

I addition, it litters your public API with extra information. The client shouldn't know or care that the Observable is immutable, that's an implementation detail that makes usages more verbose and less readable.

I suggest flipping the names, so the standard implementation is Observable and the subclass is MutableObservable. That would make the relationship clear and clean up public usages.

class SomeViewModel {
    /// Public property
    var position: Observable<CGPoint> = {
        return positionSubject
    }
    
    /// Private property
    private let positionSubject = MutableObservable(CGPoint.zero)
}
@4brunu
Copy link
Contributor

4brunu commented Mar 31, 2019

I agree!
I think we can say that this is may fault, since I introduced those names...
The reason I introduce this naming was backwards compatibility, this way the Observable would keep it's previous behaviour.
One advantage of changing the Observable to be read only, it's that this it's closer to RxSwift that it's very popular, and a lot of people is familiar with, and it would be and easier entry level to this library.
The disadvantage is that, this would require some refactoring from the current users of the library.

@4brunu
Copy link
Contributor

4brunu commented Jan 5, 2020

I just created #33 to try to fix this issue.

@4brunu
Copy link
Contributor

4brunu commented Jan 6, 2020

Since #33 was merged, I think this issue can now be closed 🙂

@roberthein
Copy link
Owner

👌

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

No branches or pull requests

3 participants