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 generic ValueType to RACSignal and RACCommand #15

Merged
merged 1 commit into from
Nov 8, 2016

Conversation

erichoracek
Copy link
Contributor

@erichoracek erichoracek commented Oct 4, 2016

See #12

I've made sure that this generic type can be bridged to SignalProducer/Signal by adding a new free function to https://github.com/ReactiveCocoa/ReactiveObjCBridge/ as a test.

I can start a pull request there as well (if desired) that integrates this pull request.

@andersio
Copy link
Member

andersio commented Oct 4, 2016

This had been discussed before, and the lightweight generics system was deemed very limited to be worthless at that time. Would you mind to show if the issues are addressed?

@erichoracek
Copy link
Contributor Author

erichoracek commented Oct 4, 2016

@andersio thanks for taking a look. I've laid out the case for this in #12. As stated there, this is primarily for adding type annotations to existing signals for clarity and improving bridging to Swift, and expressly not for having the compiler help out with type mismatches when writing signal chains in Objective-C (the issue you linked to). Let me know if you feel like any justification is missing from #12, or if that isn't enough to merit this change.

@erichoracek
Copy link
Contributor Author

As a concrete example, I can show the difference in the bridging functions that would be available with this change. Currently, RACSignalSignalProducer bridging looks like:

extension RACSignal {
    public func toSignalProducer(file: String = #file, line: Int = #line) -> SignalProducer<Any?, NSError> {
        return SignalProducer { observer, disposable in
            let next = { obj in
                observer.send(value: obj)
            }

            let failed: (_ nsError: Swift.Error?) -> () = {
                observer.send(error: ($0 as? NSError) ?? defaultNSError("Nil RACSignal error", file: file, line: line))
            }

            let completed = {
                observer.sendCompleted()
            }

            disposable += self.subscribeNext(next, error: failed, completed: completed)
        }
    }
}

However, with this change, it can be updated to the following:

public func signalProducer<Value>(from signal: RACSignal<Value>, file: String = #file, line: Int = #line) -> SignalProducer<Value?, NSError> {
    return SignalProducer<Value?, NSError> { observer, disposable in
        let next: (_ value: Value?) -> Void = { obj in
            observer.send(value: obj)
        }

        let failed: (_ nsError: Swift.Error?) -> () = {
            observer.send(error: ($0 as? NSError) ?? defaultNSError("Nil RACSignal error", file: file, line: line))
        }

        let completed = {
            observer.sendCompleted()
        }

        disposable += signal.subscribeNext(next, error: failed, completed: completed)
    }
}

Note the difference in return types—before you get a SignalProducer<Any?, NSError>, and after you get a SignalProducer<Value?, NSError>. This is a huge win in reducing the boilerplate needed for bridging, especially for those of us that have large-scale Obj-C/RAC2 codebase that are just beginning the migration to Swift, and can't afford to stop and rewrite everything in ReactiveSwift.

@mdiep
Copy link
Contributor

mdiep commented Oct 5, 2016

You still can't express the semantics of map though, right? Won't that severely limit the usefulness of generics? Any types need to be annotated very explicitly.

Since you can always opt to not annotate the type, I don't see any reason to not do this.

Have you tried using this in your codebase? I'm interested to hear how it works in practice.

@mdiep
Copy link
Contributor

mdiep commented Oct 5, 2016

You still can't express the semantics of map though, right? Won't that severely limit the usefulness of generics? Any types need to be annotated very explicitly.

I see now that you answered that in #12. I'm still interested to hear about how this been practically in your codebase.

@erichoracek
Copy link
Contributor Author

@mdiep we haven't started integrating these changes specifically into our codebase. However, we've really enjoyed the benefits of adding lightweight generics to RACCommand inputs and hope to extend them to RACSignal as well. I see the benefits as threefold:

  1. Bridging to ReactiveSwift. If you change the value type of a RACSignal in a refactor, the compiler will ensure that everywhere it is consumed uses the new type. Before this change, you would have to manually ensure that all of your Swift as casts are updated by hand. Will definitely help resolve issues in cases like these.

  2. Simplifying documentation comments. We currently have many documentation comments that look like the following:

    /// Sends the currently authenticated `User` and then completes, or else errors.
    - (RACSignal *)fetchUser;
    

    Note that the User type is encoded in the documentation comment—not the type signature of the method. If the User class was renamed as APIUser and this documentation comment was missed, the compiler would not complain, and now you'd have an out of date documentation comment that doesn't reference a real class. By migrating to lightweight generics, we can have the compiler checking that our types annotations are valid.

  3. Ensuring passed around signals have the correct type. If you're rigorous about annotating signals with the right type, if you have a method that accepts RACSignal<NSString *> *, then the compiler will make sure that you don't attempt to pass it a RACSignal<NSNumber *> *.

@mdiep
Copy link
Contributor

mdiep commented Oct 6, 2016

I'm on board. 👍 I'll try to give this a proper review soon unless someone beats me to it. (RAC 5 and Carthage are keeping me busy atm.)

Copy link
Contributor

@mdiep mdiep left a comment

Choose a reason for hiding this comment

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

These changes look great. 👍

But are these a breaking change from 1.0?

@@ -180,7 +180,7 @@ NS_ASSUME_NONNULL_BEGIN
/// Additional methods to assist with unit testing.
///
/// **These methods should never ship in production code.**
@interface RACSignal (Testing)
@interface RACSignal<__covariant ValueType> (Testing)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this sort of change required? i.e., is adding generics a breaking change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this change was not enforced by the compiler—I just added it so that asynchronousFirstOrDefault could have access to ValueType. Note that RACSignal (Debugging) does not have it as part of this change.

@erichoracek
Copy link
Contributor Author

If someone is using RACObjC in the legacy RAC2.5 way (no generics, no Swift bridging), this should not be a breaking change. However, I know of two ways that this could be a breaking change for some users:

  • ReactiveObjCBridge. When you attempt to use the current pattern of adding extensions to RACSignal to expose bridging methods, you encounter the following issue:

    Extension of a generic Objective-C class cannot access the class's generic parameters at runtime

    If there's no way around this, we'll have to add new free functions that bridge between the two (in the same style as bridgedAction. It seems this was already an issue on RACCommand, as the toAction function is commented out.

  • Existing usages of RACCommand with a generic input. Since there are now two generic types (InputType and ValueType), the compiler will now error if you only have InputType supplied. However, the migration in this case is as simple as migrating from RACCommand<NSNumber *> * to RACCommand<NSNumber *, id> *.

@mdiep
Copy link
Contributor

mdiep commented Oct 10, 2016

How would you suggest approaching this from a versioning perspective?

@erichoracek
Copy link
Contributor Author

Since there have been no changes to the implementation of ReactiveObjC, I would consider this to be a minor change to the framework. There may be some errors produced on migration, but they are easily recoverable, and only affect consumers that are using a recent API.

However, I'd suggest that this would be a major/breaking change to ReactiveObjCBridge, since we'd have to change the way that signals are currently bridged from Obj-C to Swift as part of this change.

@mdiep
Copy link
Contributor

mdiep commented Nov 8, 2016

We discussed this and decide to merge and do a 2.0 release.

@mdiep mdiep merged commit e100db8 into ReactiveCocoa:master Nov 8, 2016
@erichoracek erichoracek deleted the generics branch November 8, 2016 17:35
erichoracek added a commit to erichoracek/ReactiveObjC that referenced this pull request Nov 8, 2016
erichoracek added a commit to erichoracek/ReactiveObjC that referenced this pull request Nov 8, 2016
byohay pushed a commit to byohay/ReactiveObjC that referenced this pull request Dec 14, 2021
stuartofmine pushed a commit to stuartofmine/ReactiveObjC_2023 that referenced this pull request Oct 18, 2023
Add generic ValueType to RACSignal and RACCommand
stuartofmine pushed a commit to stuartofmine/ReactiveObjC_2023 that referenced this pull request Oct 18, 2023
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.

3 participants