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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃馃徏 Rephrase async code as PromiseKit Promises #362

Merged
merged 8 commits into from May 12, 2021
Merged

Conversation

chris-araman
Copy link
Contributor

@chris-araman chris-araman commented May 7, 2021

RE: #346 (comment)

Using Combine would seem to be the future-proof option, as that's the API Apple has produced for asynchronous programming. However, we can't use Apple's Combine yet because it requires macOS 10.15. There are some open source implementations that support older macOS releases.
馃敶 CombineX failed swift build.
馃煛 I made some progress with OpenCombine, but got stuck when I found that it doesn't yet implement merge.

So, here's the async code rephrased as PromiseKit promises.

馃憤馃徏 Pros:

  • 89 fewer LOC in mas and MasKit.
  • Arguably improved readability.
  • PromiseKit is well documented and seems to have good test coverage.

馃憥馃徏 Cons:

  • The default PromiseKit configuration assumes a UI app. MasKit.initialize configures PromiseKit for use in a console app, where we regularly wait on the main queue for promises to complete on the global queue. main.swift and all test case entry points invoke MasKit.initialize.
  • It's another dependency to maintain.
  • The mas executable (arm64, release) increases in size by ~388 KiB, or ~53%. I might be able to reduce this with further investigation, but considering we just shed ~13 MiB from MasKit.framework, I think it's probably fine.

I'm open to keeping this or dropping it. I'll leave it up to your best judgment, @phatblat.

鈩癸笍 Commit fb01f5b is here only to maintain existing behavior. With this commit, commands that download (or purchase) multiple apps will continue to attempt all downloads even if an earlier download fails. Without this commit, we'll stop downloading at the first failure. I'm inclined to think the behavior without this commit (failing fast) is what users would expect, but perhaps it's common for one download to fail while others succeed for reasons unrelated to network failures. If you agree, feel free to pop that commit off the stack, or ask me to do so, before merging.

@chris-araman chris-araman requested a review from phatblat May 7, 2021 02:33
@phatblat
Copy link
Member

phatblat commented May 7, 2021

I want to get the release out before we make this change. Also want to give time to review it properly.

@chris-araman
Copy link
Contributor Author

chris-araman commented May 9, 2021

Fixes #344. With the async logic now described in terms of promises, implementing retry logic was pretty straightforward.

caraman@mac ~/s/mas (promisekit)> swift run mas install 462062816
==> Downloading Microsoft PowerPoint
Warning: The Internet connection appears to be offline.
Trying again up to 2 more times.
==> Downloading Microsoft PowerPoint
Warning: The Internet connection appears to be offline.
Trying again up to 1 more time.
==> Downloading Microsoft PowerPoint
###################################################--------- 85.0% Installing

@chris-araman chris-araman linked an issue May 9, 2021 that may be closed by this pull request
4 tasks
Copy link
Member

@phatblat phatblat left a comment

Choose a reason for hiding this comment

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

One idea for the tests

Tests/MasKitTests/MasQuickSpec.swift Outdated Show resolved Hide resolved
@phatblat phatblat merged commit 66f89a6 into main May 12, 2021
@phatblat phatblat deleted the promisekit branch May 12, 2021 03:59
@phatblat phatblat added this to the 1.8.3 milestone Oct 19, 2021
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.

鈾伙笍 mas should retry a download if it fails due to a network timeout
2 participants