OfficeDev / teams-toolkit

Developer tools for building Teams apps
Other
466 stars 192 forks source link

TeamsFx React: create fully reactive wrapper for TeamsFx SDK #5842

Open AndrewCraswell opened 2 years ago

AndrewCraswell commented 2 years ago

Is your feature request related to a problem? Please describe. The TeamsFx React package is not fully reactive in a way that allows React components to be re-rendered when internal TeamsFx state changes. Both the TeamsFx and TeamsUserCredential classes maintain internal state. If a React component relies on that data, it won't know to re-render when the internal state changes.

This is compounded by the fact that TeamsUserCredential wraps MSAL, which itself maintains internal state (accounts, auth progress). In addition, the internal MSAL instance isn't initialized until either (a) a login is initiated or (b) a token is requested. This causes edge cases where calling getAllAccounts() on the internal MSAL instance will return undefined until a token is requested. And if later another account is added, components in the React app won't be informed that the state has changed, and won't re-render.

This is not an uncommon class of problem with React SDKs. It's the the reason the that the [MSAL React]() package was written. MSAL React uses the Context API (MsalContext) and a provider pattern (MsalProvider) to subscribe to changes to the internal MSAL state. As state changes, MSAL raises events that describe the change. The MsalProvider subscribes to the events, and updates the React state to replicate the MSAL state. The new state is passed into the MsalContext so that React app components can subscribe to the data.

It would be helpful to see the TeamsFx React SDK follow a similar pattern.

Describe the solution you'd like

A couple changes would be a huge improvement for React apps:

  1. A provider and context pattern should be followed in similar fashion to how MSAL implemented the MSAL React library. A Provider component could create instances of TeamsUserCredential, and TeamsFx. When internal state to these classes change, the provider should update React state to reflect and pass the state to a context. Components that consume the context would be able to re-render whenever data changes. The MSAL event system provides a great example for how to develop meaningful ways for frameworks (React, Vue, Svelte, etx) to subscribe to internal state change and easily create wrappers by "subscribing" to state change events. Investing in this pattern would make it easier to scale to frameworks other than React in future.
  2. A TeamsFxProvider component would make a more logical entry for configuring TeamsFx. Currently, TeamsFx seems to require that certain environment variables exist (see here). This is more an assumption that developers are using Create React App, which is limiting. If a TeamsFxProvider class existed, these configurations could be passed into the component as a prop -- thereby decoupling TeamsFx from any specific build toolchain.
  3. A low-hanging fruit would be to modify the TeamsFx class to instantiate the internal MSAL instance in the constructor. This way it's possible to interface with MSAL state before calling login() or getToken(). A reason thismight be important is because the getUserInfo() function doesn't return all the data about a user that the msal.getAllAccounts() finction would. For example, we might want to know if a user is cached, if multiple users are authentiacted, or what the user's tenantId is before attempting to get a token.
  4. Implementing a full provider/context pattern would also give a good opportunity to implement a custom useTeams hook. It's a little odd that msteams-react-base-component is a required peer dependency when it's a third party package not owned by Microsoft.

Describe alternatives you've considered One alternative that might make it easier for TeamsFx React to be reactive is to just provide integrations with the existing MSAL React library. If this were possible, then at least React apps that have been using MSAL for some time could continue to benefit from the fully reactive nature of MSAL React, without being forced to convert to TeamsFx, which is essentially a non-reactive wrapper around MSAL.

Additional context

This might make an interesting September hackathon project. I remember the MSAL React library had it's first implementation as part of a hackathon. The fast approaching hackathon next month might be a good opportunity to produce a working proof-of-concept.

ghost commented 2 years ago

Thank you for contacting us! Any issue or feedback from you is quite important to us. We will do our best to fully respond to your issue as soon as possible. Sometimes additional investigations may be needed, we will usually get back to you within 2 days by adding comments to this issue. Please stay tuned.

MuyangAmigo commented 2 years ago

Hey @AndrewCraswell , Thanks so much for your suggestion to help us improve our SDK for React developers. We will take those into planning and get back to you!

yiqing-zhao commented 2 years ago

Hi @AndrewCraswell, For points 1 and 3 regarding MSAL instance, since MSAL is a private member wrapped in the TeamsUserCredential class, I suppose that there should be no way to call getAllAccounts() directly. Also, when the app gets installed in Teams, only one account logins, and thus the scenario for Teams app might be different from MSAL regarding multiple accounts. I'm open to specific scenarios and further suggestions.

For the context and provider pattern as you suggested, we are going to add TeamsFxContext in SDK-React. This helps to reduce codes in templates and samples.

Currently, users can configure TeamsFx by using useTeamsFx(). The hook can get a custom config as input, and therefore build a customized TeamsFx instance based on the config.

Thanks a lot for your suggestion to help us with SDK-React. This means a lot to us!