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

Why is getAccounts() now asynchronous? #635

Open
joshfriend opened this issue Jun 5, 2019 · 5 comments

Comments

Projects
None yet
4 participants
@joshfriend
Copy link

commented Jun 5, 2019

The changes from #436 were only just released in 0.3.1-alpha so I'm just learning about the breaking changes now even though the commit that changed it was from back in October of last year.

Can you explain why getAccounts() is now asynchronous? This has proved to be quite annoying to migrate. Also, as noted in #548 and #544 there are still numerous cases that benefit from or even require a synchronous method of accessing this information.

If the reason is that the ADAL migration takes too long to perform on the main thread, please know that some users (hey, it me!) are not migrating from ADAL.

@heidijinxujia

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

Previously, getAccounts only retrieve the accounts in msal locally. Now, getAccounts retrieves accounts from broker as well. The broker calls need to run tasks on background threads and off the main UI.

@shoatman

This comment has been minimized.

Copy link
Contributor

commented Jun 5, 2019

@joshfriend - We're looking at how to better communicate about proposed changes to the public API. I can tell you that more are proposed. Do you have a preference/suggestion on how to provide advance notice of those changes? In terms of providing a synchronous interface as we've discussed before this is something that we can do on background threads and we're open to doing. Using the method proposed by yourself or via other library extensions/wrappers.

@NLLAPPS

This comment has been minimized.

Copy link

commented Jun 5, 2019

@joshfriend

This comment has been minimized.

Copy link
Author

commented Jun 5, 2019

Now, getAccounts retrieves accounts from broker as well

I'm not using Microsoft Authenticator or Intune Company Portal, I only want local accounts. The method that gets these is now private and using the async getAccounts() is not practical for cases where I need to know if there are any accounts logged in to update UI elements.

Do you have a preference/suggestion on how to provide advance notice of those changes?

First of all, thanks for having both a changelog file and release notes on github releases, those are very helpful. One thing I have seen other projects do successfully is to have an open issue representing the TODOs for the next release (like this).

My frustration comes from discovering that this change in particular was merged in several months before I even started using this library, but was not in jcenter as an alpha or a snapshot release. At one point during trying to fix androidx compatibility (#354 (comment)) I had tried to build the AARs locally or use git submodules but was unsuccessful and had to fall back to using the older releases from jcenter. Had I been successful in those efforts to use some kind of SNAPSHOT version, I'd have been able to figure out how to adapt to these breaking changes from the start.

So I guess I have a few suggestions:

  1. Publish SNAPSHOT releases to jcenter on merge to develop
  2. Release merged code more frequently as an alpha
  3. Have instructions somewhere in the README for how to build an AAR locally or use this repo as a git submodule (i ran into a few incorrect uses of rootProject.projectDir that prevented this)

I started using MSAL understanding that it was pre-release and that there would probably be things that get turned upside-down. I hope the above is helpful in shortening the feedback loop on new changes.

@shoatman

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

@joshfriend - Thanks for the suggestions! Very much appreciated. I'm going to publish a TODOs as you suggest shortly and look forward to your feedback. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.