search

microsoftgraph / microsoft-graph-toolkit

Authentication Providers and UI components for Microsoft Graph šŸ¦’
https://docs.microsoft.com/graph/toolkit/overview
Other
945 stars 303 forks source link

Should the `loginInitiated` event in the login component be cancelable? #1231

Closed waldekmastykarz closed 8 months ago

waldekmastykarz commented 3 years ago

According to our docs, the loginInitiated event in the login component is cancelable: https://docs.microsoft.com/en-us/graph/toolkit/components/login?context=graph%2Fapi%2F1.0&view=graph-rest-1.0#events

However, looking at our code, it seems like the cancelable property is not set and defaults to false.

https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/66a5bbb6591e6260e95dbc00c0d06bcbe8dcef38/packages/mgt-components/src/components/mgt-login/mgt-login.ts#L128

I noticed that we're checking the return value of the dispatchEvent function called internally, but according to MDN:

The return value is false if event is cancelable and at least one of the event handlers which received event called Event.preventDefault(). Otherwise it returns true.

So if I understand this correctly, unless the event object we pass into the dispatchEvent function has the cancelable property set to true, dispatchEvent would always return true.

Should we update our docs to align with the code or shall we update the code to match the docs?

The same applies by the way to the logoutInitiated event.

ghost commented 3 years ago

Hello waldekmastykarz, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback šŸ™Œ

waldekmastykarz commented 3 years ago

Happy to help with fixing the issue. Shall we update the code or the docs?

elisenyang commented 3 years ago

@waldekmastykarz we're actually planning to deprecate all of these events with the next version of the login component (https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/main/specs/mgt-login.md). The same events are available on the auth providers so are redundant.

Do you see the same issue when using these events from a provider?

waldekmastykarz commented 3 years ago

Understood. If I followed the code correctly, it seems like events in providers use a custom implementation for dispatching events, which doesn't support bubbling or cancelling at all and is just calling all registered handlers passing the event object:

https://github.com/microsoftgraph/microsoft-graph-toolkit/blob/928dfe2a4d6d4b4962f7481638146dabff1d9f57/packages/mgt-element/src/utils/EventDispatcher.ts#L29-L33

I think in the context of Provider, cancelling or bubbling doesn't make sense because events communicate state change that's already done and can't be cancelled. But I do wonder if we removed events from the Login component, how would you cancel login just based on the Provider events?

elisenyang commented 3 years ago

@waldekmastykarz can you provide some use cases/scenarios for when you would want to cancel login from the provider?

waldekmastykarz commented 3 years ago

User selected the wrong account and wants to cancel the process would be one scenario I can think of. Another could be that the login process is stuck and I want to restart it.

sebastienlevert commented 8 months ago

At the moment, this is not something we're likely to prioritize. That being said, the Graph Toolkit is an open source project and we'll be happy to support and review if you want to contribute to its codebase! In the meantime, we will be closing this issue. Thanks!