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

Make non-class types be able to conforms to ReactiveExtensionProvider #636

Merged

Conversation

ra1028
Copy link
Contributor

@ra1028 ra1028 commented Apr 27, 2018

This is very tiny changes.

Checklist

  • Updated CHANGELOG.md.

@ra1028 ra1028 force-pushed the feature/non-classtype-reactive-extension branch from 9959727 to c717fdb Compare April 27, 2018 10:09
@mdiep
Copy link
Contributor

mdiep commented Apr 27, 2018

I know there's been some discussion of removing this requirement before. Can you share an example of a non-class type where you'd like to use it?

@ra1028
Copy link
Contributor Author

ra1028 commented Apr 27, 2018

e.g, want to use reactive extension for another extension host.
Do we need to restrict to class type?

public protocol ExtensionsProvider: class {}

public extension ExtensionsProvider {
    var extensions: Extension<Self> {
        return Extension(self)
    }
    
    static var extensions: Extension<Self>.Type {
        return Extension<Self>.self
    }
}

public struct Extension<Base> {
    public let base: Base
    
    fileprivate init(_ base: Base) {
        self.base = base
    }
}

extension UIView: ExtensionsProvider {}

public extension Extension where Base: UIView {
    func fooString() -> String {
        return "foo"
    }
}

extension Extension: ReactiveExtensionsProvider {}

public extension Reactive where Base == Extension<UIView> {
    func fooString() -> SignalProducer<String, NoError> {
        return SignalProducer(value: base.fooString())
    }
}

@mdiep
Copy link
Contributor

mdiep commented Apr 30, 2018

I'm not sure this is something that we want to support.

Reactive extensions only really make sense with a reference type. If you were trying to change a struct or enum, you'd have problems. I'm not sure if enabling other wrappers like this is worth it given that it might be confusing to users.

@ReactiveCocoa/reactivecocoa?

@sharplet
Copy link
Contributor

I have had this particular use case before when writing extensions for Kingfisher, which uses a .kf extension provider for its UIKit wrapper functionality. The value returned from .kf is a class, which explains why I didn't have this limitation.

I'm not currently opposed to loosening the restriction, although as you say it doesn't make a lot of sense for binding targets. @mdiep is that the confusion you're thinking of? That users might make a binding target to a struct and then be confused that it doesn't mutate shared state?

@mdiep
Copy link
Contributor

mdiep commented Apr 30, 2018

@mdiep is that the confusion you're thinking of? That users might make a binding target to a struct and then be confused that it doesn't mutate shared state?

Yup.

@andersio
Copy link
Member

andersio commented May 14, 2018

That users might make a binding target to a struct and then be confused that it doesn't mutate shared state?

This is unlikely to happen since BindingTarget does not support inout reference. So if the host is value type, mutations attempted in the closure would be blocked by the compiler (unless the enclosing declaration has the mutating modifier).

Either way, this is a semantic error that IMO we shouldn't account for.

@ra1028
Copy link
Contributor Author

ra1028 commented May 18, 2018

Either way, this is a semantic error that IMO we shouldn't account for.

Agreed.

@mdiep
How about?

@mdiep mdiep merged commit 29fb36d into ReactiveCocoa:master May 21, 2018
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.

None yet

4 participants