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

Crash iOS app when throwing error on kotlin flow #818

Merged
merged 6 commits into from
Aug 27, 2024

Conversation

shin-usu
Copy link
Contributor

@shin-usu shin-usu commented Aug 26, 2024

Issue

  • close #ISSUE_NUMBER

Overview (Required)

When error is thrown on kotlin flow, iOS app crash.
According to this article, Skie does not propagate errors that occur in Flow to the iOS side.
https://touchlab.co/skie-migration?ti=44D6E96AB6894C3C883219A3B4#flow-support

Therefore, I added error catch logic to kotlin flow.
In fact, I would like to pass the Result type to iOS for error handling on the iOS side, but since the type would be AnyObject, our priority at this stage is to avoid crashes.

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

Before After

Movie (Optional)

Before After

Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 26, 2024 16:06 Inactive
@shin-usu shin-usu force-pushed the feature/ios-flow-crash branch from 2486ce4 to 7e9a7c5 Compare August 26, 2024 17:01
Copy link

Detekt check failed. Please run ./gradlew detekt --auto-correct to fix the issues.

@shin-usu shin-usu force-pushed the feature/ios-flow-crash branch from 7e9a7c5 to ba6ace7 Compare August 26, 2024 17:03
@github-actions github-actions bot temporarily deployed to deploygate-distribution August 26, 2024 17:07 Inactive
@shin-usu shin-usu force-pushed the feature/ios-flow-crash branch from ba6ace7 to f2a7e76 Compare August 26, 2024 17:09
@github-actions github-actions bot temporarily deployed to deploygate-distribution August 26, 2024 17:11 Inactive
@shin-usu shin-usu changed the title [WIP] Crash iOS app when throwing error on kotlin flow Crash iOS app when throwing error on kotlin flow Aug 26, 2024
@shin-usu shin-usu marked this pull request as ready for review August 26, 2024 17:13
@takahirom
Copy link
Member

It seems this is the issue: touchlab/SKIE#19.
Currently, the users of Flow are only on iOS because Android is using the Composable function. I'm fine with this change.

@@ -36,6 +38,10 @@ public class DefaultContributorsRepository(
refresh()
}
}
.catch {
Logger.e("Fail getContributorStream: $it")
Copy link
Member

Choose a reason for hiding this comment

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

I think we can pass throwable to Logger.e parameter 👀

@@ -36,6 +38,10 @@ public class DefaultContributorsRepository(
refresh()
}
}
.catch {
Copy link
Member

Choose a reason for hiding this comment

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

How about adding a comment for each .catch block?

// SKIE doesn't support throwing exceptions from Flow.
// For more information, please refer to https://github.com/touchlab/SKIE/discussions/19 .

if (first) {
first = false
Logger.d("DefaultSessionsRepository onStart getTimetableStream()")
sessionCacheDataStore.save(sessionsApi.sessionsAllResponse())
Copy link
Member

@takahirom takahirom Aug 27, 2024

Choose a reason for hiding this comment

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

The sessionsApi.sessionsAllResponse is likely to throw an error.

try {
sessionCacheDataStore.save(sessionsApi.sessionsAllResponse())
} catch (e: Exception) {
// SKIE doesn't support throwing exceptions from Flow.
// For more information, please refer to https://github.com/touchlab/SKIE/discussions/19 .
coroutineContext.ensureActive()
Logger.e(e, "Fail to refresh Timetable in getTimetableStream()")
}

@shin-usu
Copy link
Contributor Author

@takahirom
I don't know much about Kotlin, so thanks for the helpful comments 🙏
I'll fix those later.

@github-actions github-actions bot temporarily deployed to deploygate-distribution August 27, 2024 14:06 Inactive
@shin-usu
Copy link
Contributor Author

@takahirom
I fixed for your suggestions. Could you please re-review??

@shin-usu shin-usu requested a review from takahirom August 27, 2024 14:18
Copy link
Member

@takahirom takahirom left a comment

Choose a reason for hiding this comment

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

Looks good!

@shin-usu shin-usu merged commit 17d92cc into main Aug 27, 2024
7 checks passed
@shin-usu shin-usu deleted the feature/ios-flow-crash branch August 27, 2024 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants