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

feat(core): add IPC channel #6813

Merged
merged 25 commits into from
May 11, 2023
Merged

feat(core): add IPC channel #6813

merged 25 commits into from
May 11, 2023

Conversation

lucasfernog
Copy link
Member

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

@lucasfernog lucasfernog requested a review from a team as a code owner April 27, 2023 20:11
Copy link
Member

@wusyong wusyong left a comment

Choose a reason for hiding this comment

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

Is this mobile only feature?

@lucasfernog
Copy link
Member Author

Is this mobile only feature?

I've also implemented it for desktop (it can easily be backported to v1) though the main idea here is to use it to send events from the Kotlin and Swift sides to the webview (eventually we'll also need a communication channel between mobile and Rust, but that's a little complicated since it involves static variables).

@JonasKruckenberg
Copy link
Member

Is this part of the public API?

@lucasfernog
Copy link
Member Author

Yeah it is part of the public API @JonasKruckenberg

@lucasfernog lucasfernog requested a review from chippers May 2, 2023 19:26
Copy link
Member

@chippers chippers left a comment

Choose a reason for hiding this comment

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

I'm not fully up to date with mobile, but I see what you are trying to do with the channel communication here.

Only suggestion would be to cut the channel prefix once, instead of splitting on the string and grabbing the second.

chippers
chippers previously approved these changes May 6, 2023
chippers
chippers previously approved these changes May 6, 2023
@JonasKruckenberg
Copy link
Member

Can we hold off on merging this until I had the chance to review it tomorrow?

@lucasfernog
Copy link
Member Author

Sure @JonasKruckenberg

Comment on lines 54 to 60
if value.starts_with(CHANNEL_PREFIX) {
let callback_id: Option<usize> = value
.chars()
.skip(CHANNEL_PREFIX.len())
.collect::<String>()
.parse()
.ok();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use value.split_once(CHANNEL_PREFIX)

Copy link
Member Author

Choose a reason for hiding this comment

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

Pushed.

@@ -55,6 +55,29 @@ function transformCallback(
return identifier
}

class Channel {
Copy link
Member

Choose a reason for hiding this comment

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

I would refactor this to be more similar to the Websocket or BroadcastChannel api i.e. the constructor takes no arguments and you can listen to events by overriding the on message property or calling .addEventListener('message', () => {})

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, pushed.

@lucasfernog lucasfernog merged commit 0ab5f40 into next May 11, 2023
@lucasfernog lucasfernog deleted the feat/channel branch May 11, 2023 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔎 In audit
Development

Successfully merging this pull request may close these issues.

4 participants