Skip to content

Uniffi push component#4431

Merged
tarikeshaq merged 8 commits intomainfrom
uniffi-push-step2
Sep 13, 2021
Merged

Uniffi push component#4431
tarikeshaq merged 8 commits intomainfrom
uniffi-push-step2

Conversation

@tarikeshaq
Copy link
Copy Markdown
Contributor

@tarikeshaq tarikeshaq commented Sep 3, 2021

Fixes #4424

The commits in order:

  • adds a valid UDL
  • Rust compiles with scaffolding included
  • Removes ffi crate
  • Removes protobuf types and replaces them with rust types
  • Adds kotlin bindings and cleans up the API to allow android to consume this

This is ready for review 🚀 Before this merges though, a couple of TODOS:

@tarikeshaq tarikeshaq requested a review from a team September 3, 2021 17:53
Comment thread components/push/src/push.udl Outdated
Comment thread components/push/src/push.udl Outdated
constructor(string sender_id, optional string server_host = "updates.push.services.mozilla.com", optional string http_protocol = "https", string bridge_type, string registration_id, optional string database_path = "push.sqlite");

[Throws=PushError]
SubscriptionResponse subscribe([ByRef] optional string channel_ID = "", [ByRef] optional string scope = "",[ByRef] optional string? appServerKey = null);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[ByRef] optional string? appServerKey = null is a handful 😅 but it's the closest representation of the type used there.

Comment thread components/push/src/push.udl Outdated
@tarikeshaq tarikeshaq changed the base branch from main to uniffi-push-move-to-internal September 4, 2021 18:36
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 4, 2021

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.71%. Comparing base (a447079) to head (d6bf2e4).
⚠️ Report is 2819 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4431   +/-   ##
=======================================
  Coverage   74.71%   74.71%           
=======================================
  Files          47       47           
  Lines        4157     4157           
=======================================
  Hits         3106     3106           
  Misses       1051     1051           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Base automatically changed from uniffi-push-move-to-internal to main September 7, 2021 17:08
@tarikeshaq tarikeshaq force-pushed the uniffi-push-step2 branch 2 times, most recently from 38e4c6d to 821ef2b Compare September 7, 2021 17:54
@tarikeshaq tarikeshaq marked this pull request as ready for review September 7, 2021 23:02
@@ -141,12 +141,12 @@ class PushTest {

Copy link
Copy Markdown
Contributor Author

@tarikeshaq tarikeshaq Sep 7, 2021

Choose a reason for hiding this comment

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

The changes in this file are the extent of the breaking changes...

  • A-C will have to turn the List<Byte> into a ByteArray (what decrypt used to return) if they'd like using toByteArray. It was either this, or have a small wrapper to do the conversion, I opted for the "less android code in a-s" approach
  • channelID -> channelId: this is trivial but interesting, I tried setting it to channel_ID, but the rust compiler gave me warnings to turn it into channel_id to make it snake case. So either we make the breaking change, or we can silence the rust compiler... I almost opted for the latter, but wanted to get a second opinion
  • PushError -> PushException

Copy link
Copy Markdown
Member

@mhammond mhammond left a comment

Choose a reason for hiding this comment

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

Great work Tarik - very thorough and well thought out! Please do complete those other todos before merging though - I'm worried about overloading the android-components people, so think we should have a PR up before merging.

But great work, thanks!

Comment thread Cargo.toml

// NOTE: this returns a `Vec<i8>` since the kotlin consumer is expecting
// signed bytes.
Ok(decrypted.into_iter().map(|ub| ub as i8).collect())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this related to unsigned types being experimental in Kotlin? If so, I think that's no longer the case. (IOW, if it's possible to now use u8 here I think we should do so, but I've no idea how practical that is, so feel free to ignore!)

Copy link
Copy Markdown
Contributor Author

@tarikeshaq tarikeshaq Sep 8, 2021

Choose a reason for hiding this comment

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

Uhh yes, I used this because Android components expects a ByteArray, which is just one .toByteArray() away from a List<Byte>, but List<UByte> can't convert directly. So A-C will have to convert the List<UByte> to ByteArray through a UByteArray or a List<Byte>... which isn't that bad, but it would be the first time they interact with unsigned types over there 😅 I guess it's just a matter of do we want to do the conversion for them, or have them do the conversion themselves

@tarikeshaq
Copy link
Copy Markdown
Contributor Author

tarikeshaq commented Sep 8, 2021

Thanks Mark!! I created a pr on A-C to get feedback mozilla-mobile/android-components#10953

Correct me if I'm wrong, but from yesterday's discussion, we should cut a release before this merges, resolve the other breaking changes, then cut another release for this right?

Edit: Yup, that's what we're doing, we already cut 83.0.0! I'll wait for that to make its way to AC

@tarikeshaq
Copy link
Copy Markdown
Contributor Author

This should be in a mergeable state! I'm planning to merge it sometime early next week and cut another major release. Once that's done I'll update mozilla-mobile/android-components#10953 to have the latest release. (cc: @mhammond and @jonalmeida, no action items, just letting you know my plan here in case you have any concerns or thoughts 😄)

@tarikeshaq tarikeshaq merged commit 7a1533b into main Sep 13, 2021
@tarikeshaq tarikeshaq deleted the uniffi-push-step2 branch September 13, 2021 21:47
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.

Implement uniffi'ing the push component

3 participants