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

[iOS] Add sendable conformance to event map feature #965

Merged
merged 6 commits into from
Sep 10, 2024

Conversation

woxtu
Copy link
Contributor

@woxtu woxtu commented Sep 5, 2024

Issue

Overview (Required)

  • This PR adds sendable conformance to the event map feature and resolves the following warning:
Passing argument of non-sendable type 'EventMapReducer.Action' into main actor-isolated context may introduce data races

Links

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

Before After

Movie (Optional)

Before After

Copy link
Member

@ry-itto ry-itto left a comment

Choose a reason for hiding this comment

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

Thanks!
Please check a comment 🙏🏼

public let name: MultiLangText
public let roomName: MultiLangText
public let roomIcon: RoomIcon
public let description_: MultiLangText
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 this _ isn't needed for iOS side code 👀

Suggested change
public let description_: MultiLangText
public let description: MultiLangText

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed it.

Copy link
Contributor

@MrSmart00 MrSmart00 left a comment

Choose a reason for hiding this comment

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

Thanks for your commits!
I left some comments.
Please check them🙏

@@ -2,9 +2,10 @@ import SwiftUI
import CommonComponents
import Theme
import shared
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this import with your modified?
I guess Model package wrapped shared package, it makes shared imports to be no needed.

@@ -2,7 +2,7 @@ import ComposableArchitecture
import KMPClient
import Model
import Foundation
@preconcurrency import shared
import shared
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this import with your modified?
I guess Model package wrapped shared package, it makes shared imports to be no needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point!

Copy link
Contributor

@MrSmart00 MrSmart00 left a comment

Choose a reason for hiding this comment

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

Thanks your commits!!!
LGTM 🙏

@ry-itto ry-itto merged commit a8e57b3 into DroidKaigi:main Sep 10, 2024
4 checks passed
@woxtu woxtu deleted the sendable-event-map branch September 13, 2024 17:35
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.

3 participants