Deprecate Promises, remove AsyncTests #5511
Conversation
gbirch-stripe
changed the title
StripePayments- public var allResponseFields: [Swift.AnyHashable : Any] {
- get
- }
- public var allResponseFields: [Swift.AnyHashable : Any] {
- get
- }
- public var allResponseFields: [Swift.AnyHashable : Any] {
- get
- }If you are adding a new public API consider the following:
If you are modifying or removing a public API:
If you confirm these APIs need to be added/updated and have undergone necessary review, add the label ℹ️ If this comment appears to be left in error, make sure your branch is up-to-date with |
gbirch-stripe
requested review from
a team,
joyceqin-stripe,
davidme-stripe,
wooj-stripe and
porter-stripe
and removed request for
a team
| } | ||
|
|
||
| func testFutureChainedFailure() { | ||
| let promise = Promise<Int>() |
There was a problem hiding this comment.
Are promises still being used in the code base anywhere? Even though we don't want people to use them in the future and the tests are flakey I'm a little fud-ful of removing the tests.
There was a problem hiding this comment.
They're used in financial connections and identity, and I've notified both teams of this. The issue with keeping the tests around is that some of the scenarios they test are indeed broken, and the only reason they usually pass is that we're not getting unlucky with the timing. So between failing all the time and only passing because they're not accurate, I'm not sure what the purpose of them is
There was a problem hiding this comment.
cc @mats-stripe can you review and see if this looks good as only FC and Identity use promises?
| @@ -123,6 +124,7 @@ import Foundation | |||
| } | |||
| } | |||
|
|
|||
| // ⛔️ DEPRECATED: Promises are not fully thread safe and can cause crashes. Use Swift concurrency instead. ⛔️ | |||
| @_spi(STP) public class Promise<Value>: Future<Value> { | |||
There was a problem hiding this comment.
The comments may get overlooked or not seen, one idea is to move this from STP to something like "DeprecatedPromises" or something so future users need to ack that they are deprecated if they start using them in a place where they aren't currently used.
There was a problem hiding this comment.
I tinkered with this but decided against it since the only reason I can imagine someone writing new Promise code is to interact with existing Promise code, which there's no way around right now due to lack of interoperability. So my feeling was that it was add friction without benefit, but I think I still have the commit somewhere so I'm curious to hear your thoughts
|
Tracking removal of Promise code: |
Summary
Motivation
Ensure that no new promise code is added to the SDK
Remove CI friction
Testing
N/A
Changelog
N/A