Pitch: Deprecate ActionProtocol #248

sharplet · 2017-02-05T04:22:21Z

...instead of removing it completely. See the discussion on #245 for context. I'm proposing this change as an alternative for discussion: there are some issues with this approach, so I thought it would be good to have something concrete to discuss.

Benefits:

Mergeable into master now, suitable for inclusion in the next minor release.
ActionProtocol is an implementation detail, but isn't clearly documented as such. There are possibly users making use of it in ways we didn't intend (I was once one of them).

Update: See #248 (comment).

Downsides:

Because conditionally compiled code has to be syntactically valid, we essentially have to duplicate the extension. I experimented with using an internal typealias to work around this limitation, but it was unfortunately a dead-end.

I don't have much of a problem with the amount of duplication in this case. But it's pretty clear to me that this approach won't scale all that well to SignalProtocol and SignalProducerProtocol, whose extensions currently house all of the operator definitions. There would be a lot of duplication. There are certainly techniques we could use to avoid duplicating the implementations (reducing the duplication to declarations only), but they might not be pretty.

sharplet · 2017-02-05T04:28:13Z

Although I just realised something: given that 3.1 is supposed to be source-compatible with 3.0, I think we should be able to just slap the deprecation warning on and not duplicate anything?? I'll take a look at how to get Swift 3.1 to compile in 3.0 mode...

sharplet · 2017-02-05T04:35:39Z

I've just pushed a change which removes the duplication.

sharplet · 2017-02-05T04:38:51Z

I now believe this should work just fine for our other *Protocols, too. I'll do some more experimenting tomorrow.

sharplet · 2017-02-05T04:43:46Z

... suitable for inclusion in the next minor release

Because the deprecation warning only shows for Swift >= 3.1, this could now go into a patch release.

mdiep · 2017-02-05T05:03:42Z

Not sure what to do about the podspec error. I guess we should --allow-warnings?

sharplet · 2017-02-05T05:12:41Z

Interesting, I didn't see that warning in my tests. I'll try with a different Xcode version and see if it comes up (I was using the 8.3 beta).

sharplet · 2017-02-05T05:25:45Z

@available(swift, ...) is only, ahem, available, in Swift 3.1: https://github.com/apple/swift-evolution/blob/master/proposals/0141-available-by-swift-version.md. I'll have to nut out how to handle this properly under semver. Though if this is just going to emit a warning when compiling ReactiveSwift and not in user code, it might be fine to ignore it.

andersio · 2017-02-05T10:32:45Z

This is not source compatible since the definition has changed. Even if it is kept intact, the real problem is that we cannot do this with SignalProtocol and SignalProducerProtocol, while ideally we might want to do all these in one batch. Once we transfer the default implementations to the concrete type, existing external extensions to these two protocols would lose accesses to all those operators, unless we make trampolines.

Another issue would be the deprecation warnings in our codebases that cannot be suppressed, should we still have any extensions to these deprecated entities.

sharplet · 2017-02-05T17:00:56Z

Yeah, we don't have to keep the incompatible changes, this should probably just be the deprecation. As the implementations have no longer moved, I'm not sure I see why we can't do this with the other protocols?

andersio · 2017-02-05T18:53:51Z

If it is just marking it ~~unavailable~~ deprecated, it should be okay for a point release. Just that for other protocols, specifically SignalProtocol and SignalProducerProtocol, AFAIU our codebase would get tons of deprecation warnings on the protocol extensions that cannot be suppressed.

sharplet · 2017-02-05T19:09:58Z

I also just realised that it's not going to be possible to remove SignalProtocol and SignalProducerProtocol until we get recursive constraints in extensions (used for the flatten operator), which won't land until at least the Swift 4 timeframe. I don't think it would make sense to add a deprecation warning for these protocols until we have a version of that feature we can test with.

So, the current state of this PR:

On Swift 3.1, this change emits deprecation warnings for all uses of ActionProtocol
On Swift 3.0.x, this change emits an "invalid platform" warning in ReactiveSwift, but doesn't emit any warnings in calling code.

It's a shame that there's no way to suppress the deprecation warnings for uses internal to ReactiveSwift, and they're a little annoying. But I don't think I mind that for the sake of users. @ReactiveCocoa/reactiveswift I'd love to hear your thoughts on the matter of warnings internal to this codebase. It doesn't bother me, but I totally understand if others feel differently.

mdiep · 2017-02-06T02:44:01Z

I don't mind internal deprecation warnings—so long as the next major release isn't far off.

If we really feel this is a pain, we could do it right before the last 1.x release.

andersio · 2017-02-06T06:20:15Z

I think recursive constraint is not relevant here, but parameterized extensions. ~~Sadly, it seems the 2.0 plan has to be scrapped then.~~ An alternative plan is to move everything except those that cannot (flattening, optionalize, etc), while keeping the protocols. Then we leave the remaining work to RAS 3.0. This way, we still have all operators hosted in the concrete type. That's said there is no guarantee that we would have parameterized extensions in Swift 4.0.

sharplet · 2017-02-06T14:20:31Z

Maybe both? This definitely fails under Swift 3.1 with a recursive constraint error:

// Flatten.swift
extension Signal where Value == Signal ...

andersio · 2017-02-06T14:34:49Z

Hmm, perhaps.

By the way, I have a branch with producer operators converted, e.g.

extension SignalProducer where Value: _SignalProducerProtocol, Error == Value.Error {

But it got hit by an issue in overloading somehow specific to concrete type extensions, and it so far affects attempt and then.

Bug Report: https://bugs.swift.org/browse/SR-3873

Swift 3.1 now raises an error when using a private variable from an inlined function.

andersio · 2017-02-15T14:21:13Z

Sources/Action.swift

@@ -207,6 +207,7 @@ private struct ActionState {
 }

 /// A protocol used to constraint `Action` initializers.
+@available(swift, deprecated: 3.1, message: "This protocol is no longer necessary and will be removed in a future version of ReactiveSwift. Use Action directly instead.")


Since it cannot be suppressed anyway, we may deprecate it for 3.0.x too.

I'm not so sure: currently it only generates a warning inside ReactiveSwift, so I like the idea that it won't generate any warnings for consumers.

This would happen when building through Carthage in Swift 3.1 anyway... https://travis-ci.org/ReactiveCocoa/ReactiveSwift/jobs/201869272#L821

sharplet · 2017-02-15T18:35:26Z

Yeah, I feel like that's better than introducing deprecation warnings into consuming projects unnecessarily.

…

On Feb 15, 2017, at 1:30 PM, Anders Ha ***@***.***> wrote: @andersio commented on this pull request. In Sources/Action.swift <#248>: > @@ -207,6 +207,7 @@ private struct ActionState { } /// A protocol used to constraint `Action` initializers. ***@***.***(swift, deprecated: 3.1, message: "This protocol is no longer necessary and will be removed in a future version of ReactiveSwift. Use Action directly instead.") This would happen when building through Carthage. https://travis-ci.org/ReactiveCocoa/ReactiveSwift/jobs/201869272#L821 <https://travis-ci.org/ReactiveCocoa/ReactiveSwift/jobs/201869272#L821> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#248>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJA5m3xaTumCRyRyoREIgcgk9Gx88GAks5rc0RDgaJpZM4L3YjH>.

andersio · 2017-02-15T18:36:06Z

@sharplet Oops, it still appears in the build log.
https://travis-ci.org/ReactiveCocoa/ReactiveSwift/jobs/199639595#L826

andersio · 2017-02-15T19:03:15Z

Tried -suppress-warnings in 6eb9671 and it works.

andersio · 2017-02-19T18:55:19Z

@sharplet With #272 merged, could you please rebase it on master and strip the swift(>=3.1) constraint of the deprecation?

sharplet mentioned this pull request Feb 5, 2017

Remove ActionProtocol. #245

Closed

mdiep approved these changes Feb 5, 2017

View reviewed changes

sharplet force-pushed the as-deprecate-action-protocol branch from 2e32503 to 0c9a5ef Compare February 5, 2017 18:53

sharplet added 2 commits February 8, 2017 10:13

Deprecate ActionProtocol for swift(>=3.1)

a54f500

Fix Swift 3.1 compile error for inlined private variable

cdc3d25

Swift 3.1 now raises an error when using a private variable from an inlined function.

sharplet force-pushed the as-deprecate-action-protocol branch from 0c9a5ef to cdc3d25 Compare February 8, 2017 15:13

andersio approved these changes Feb 15, 2017

View reviewed changes

andersio reviewed Feb 15, 2017

View reviewed changes

andersio mentioned this pull request Feb 18, 2017

Suppress all warnings when building with CocoaPods. #272

Merged

andersio added blocked and removed blocked labels Feb 18, 2017

andersio mentioned this pull request Feb 27, 2017

Deprecate ActionProtocol. #284

Merged

andersio merged commit cdc3d25 into master Feb 27, 2017

andersio deleted the as-deprecate-action-protocol branch February 27, 2017 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pitch: Deprecate ActionProtocol #248

Pitch: Deprecate ActionProtocol #248

sharplet commented Feb 5, 2017 •

edited

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

mdiep commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

andersio commented Feb 5, 2017 •

edited

sharplet commented Feb 5, 2017

andersio commented Feb 5, 2017 •

edited

sharplet commented Feb 5, 2017

mdiep commented Feb 6, 2017

andersio commented Feb 6, 2017 •

edited

sharplet commented Feb 6, 2017

andersio commented Feb 6, 2017 •

edited

andersio Feb 15, 2017

sharplet Feb 15, 2017

andersio Feb 15, 2017 •

edited

sharplet commented Feb 15, 2017 via email

andersio commented Feb 15, 2017 •

edited

andersio commented Feb 15, 2017

andersio commented Feb 19, 2017

Pitch: Deprecate ActionProtocol #248

Pitch: Deprecate ActionProtocol #248

Conversation

sharplet commented Feb 5, 2017 • edited

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

mdiep commented Feb 5, 2017

sharplet commented Feb 5, 2017

sharplet commented Feb 5, 2017

andersio commented Feb 5, 2017 • edited

sharplet commented Feb 5, 2017

andersio commented Feb 5, 2017 • edited

sharplet commented Feb 5, 2017

mdiep commented Feb 6, 2017

andersio commented Feb 6, 2017 • edited

sharplet commented Feb 6, 2017

andersio commented Feb 6, 2017 • edited

andersio Feb 15, 2017

Choose a reason for hiding this comment

sharplet Feb 15, 2017

Choose a reason for hiding this comment

andersio Feb 15, 2017 • edited

Choose a reason for hiding this comment

sharplet commented Feb 15, 2017 via email

andersio commented Feb 15, 2017 • edited

andersio commented Feb 15, 2017

andersio commented Feb 19, 2017

sharplet commented Feb 5, 2017 •

edited

andersio commented Feb 5, 2017 •

edited

andersio commented Feb 5, 2017 •

edited

andersio commented Feb 6, 2017 •

edited

andersio commented Feb 6, 2017 •

edited

andersio Feb 15, 2017 •

edited

andersio commented Feb 15, 2017 •

edited