Skip to content

Conversation

AadityaDhingra
Copy link

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@jonalmeida jonalmeida self-requested a review August 6, 2025 19:42
Copy link
Collaborator

@jonalmeida jonalmeida left a comment

Choose a reason for hiding this comment

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

Self note: Leaving my offline feedback here to come back to it later.

*/
fun beginOAuthFlow(
scopes: Array<String>,
entrypoint: String,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should expose the service param here and use the default value from here since there is this thin wrapper around the rust generated FFI code.

Suggested change
entrypoint: String,
entrypoint: String,
services: List<String> = listOf("sync"),

string begin_oauth_flow([ByRef] sequence<string> scopes, [ByRef] string entrypoint);


string begin_oauth_flow([ByRef] sequence<string> scopes, [ByRef] string entrypoint, [ByRef] sequence<string> service);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use default values for the udl so that the iOS tests don't fail if that's a preference: https://mozilla.github.io/uniffi-rs/latest/proc_macro/functions.html#default-values

Alternatively, we can also fix the breaking tests too. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults in UDL are done slightly differently: https://mozilla.github.io/uniffi-rs/latest/udl/functions.html. I think a list of strings should be supported, but I'm not totally sure.

Copy link
Collaborator

@jonalmeida jonalmeida Aug 12, 2025

Choose a reason for hiding this comment

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

Yeah, sorry I realized too late that I shared the wrong doc link. 🤦

string begin_oauth_flow([ByRef] sequence<string> scopes, [ByRef] string entrypoint);


string begin_oauth_flow([ByRef] sequence<string> scopes, [ByRef] string entrypoint, [ByRef] sequence<string> service);
Copy link
Contributor

Choose a reason for hiding this comment

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

Defaults in UDL are done slightly differently: https://mozilla.github.io/uniffi-rs/latest/udl/functions.html. I think a list of strings should be supported, but I'm not totally sure.

// Allow both &[String] and &[&str] since UniFFI can't represent `&[&str]` yet,
scopes: &[T],
entrypoint: &str,
service: &[String],
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be &[T] as well, using the same hack as scopes. I think that makes some of the Rust code a bit more idiomatic, but it's not a big deal. Both scopes and service would need to be the same type (either &str or &String) but that seems okay to me.

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.

3 participants