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

feat: update @azure/msal-angular to Angular 13 and rxjs 7 #4599

Closed
wants to merge 2 commits into from
Closed

feat: update @azure/msal-angular to Angular 13 and rxjs 7 #4599

wants to merge 2 commits into from

Conversation

kyubisation
Copy link

This PR updates @azure/msal-angular to Angular 13, which switches the
library/package format from ViewEngine to Ivy. The new package format
also drops UMD bundles and instead provides (F)ESM 2015 and 2020
variants.
This also enables strict mode in the tsconfig.json for
@azure/msal-angular. Due to this the return type of the method
handleRedirectObservable in the class MsalService was changed
from Observable<AuthenticationResult> to
Observable<AuthenticationResult | null>, which is the correct return
type according to the interface and implementation.
Further rxjs was updated from version 6 to 7, while retaining backward
compatibility with version 6. The peer dependency now allows either
rxjs v6 or v7.

Closes #4154, closes #4298

This PR updates @azure/msal-angular to Angular 13, which switches the
library/package format from ViewEngine to Ivy. The new package format
also drops UMD bundles and instead provides (F)ESM 2015 and 2020
variants.
This also enables strict mode in the tsconfig.json for
@azure/msal-angular. Due to this the return type of the method
`handleRedirectObservable` in the class `MsalService` was changed
from `Observable<AuthenticationResult>` to
`Observable<AuthenticationResult | null>`, which is the correct return
type according to the interface and implementation.
Further rxjs was updated from version 6 to 7, while retaining backward
compatibility with version 6. The peer dependency now allows either
rxjs v6 or v7.

Closes #4154, closes #4298
@ghost
Copy link

ghost commented Mar 11, 2022

CLA assistant check
All CLA requirements met.

@jasonnutter
Copy link
Contributor

jasonnutter commented Mar 11, 2022

@kyubisation Thanks for the contribution. Please note, there is already a PR open with proposed (#4467) changes for updating to support Angular 13 and rxjs v7, which we are currently evaluating. Furthermore, we are looking at if/how we can make supporting rxjs v7 backwards compatible with Angular 9-12 and rxjs v6, given those are the versions we currently support and dropping those would require a major version bump.

@kyubisation
Copy link
Author

Further rxjs was updated from version 6 to 7, while retaining backward compatibility with version 6. The peer dependency now allows either rxjs v6 or v7.

I don't particularly mind which PR gets merged. Due to our company's frustrations with @azure/msal-angular (we've been in contact with Microsoft/Azure AD, I'll provide details if desired) I wanted to check how much effort is required to upgrade to Angular 13, since no roadmap was provided for that change.

Also please note that technically a backwards compatible change is not possible with the upgrade to Angular 13, since the package format changed as described.

@kyubisation
Copy link
Author

@jasonnutter As a side note; I missed the other PR. Great work by @sandrooco.
Unfortunately as it stands it will break consumers with rxjs 6, as the imports were changed in a way that is not backwards compatible. Leaving the imports as they previously were works in both v6 and v7.
Additionally ng update introduced backwards compatible test teardown logic, which is not necessary for this project. Destroying the context after each test works fine.
Apart from that, due to configuring strict mode, I noticed an error in the MsalService typings as described above and fixed it accordingly.

@jasonnutter
Copy link
Contributor

Leaving the imports as they previously were works in both v6 and v7.

Thanks for the insight on this, was not immediately clear this was the case. We've put a PR to add Angular 13 sample apps and update the peer dependency range for to include version 7. #4605

Also please note that technically a backwards compatible change is not possible with the upgrade to Angular 13, since the package format changed as described.

Testing the above PR appears to show that just updating the peer dep to include v7 is enough, and that Angular will support packages packaged in the "legacy" format. Assuming that is the case, we will continue to do that, given we do not want to publish a breaking change of MSAL Angular (given that requires a lot more work on our side outside of just the code, e.g. documentation). Once we get the above PR merged we can incrementally look at supporting an Ivy distribution in a backwards compatible way.

Appreciate the insight and patience, thanks!

@kyubisation
Copy link
Author

Testing the above PR appears to show that just updating the peer dep to include v7 is enough, and that Angular will support packages packaged in the "legacy" format.

Correct, with the ngcc (Angular Compatibility Compiler). This however requires ngcc to run every time dependencies are installed fresh (to convert ViewEngine packages to the Ivy format).

Assuming that is the case, we will continue to do that, given we do not want to publish a breaking change of MSAL Angular (given that requires a lot more work on our side outside of just the code, e.g. documentation).

Understandable, although from a consumer viewpoint very little changes. The (breaking) changes are only internal i.e. the package format. The API does not change (for the MSAL subset).

Once we get the above PR merged we can incrementally look at supporting an Ivy distribution in a backwards compatible way.

Again, this is not really possible due to the way the Angular Package Format works. It is either a ViewEngine distribution (umd, es5 (dropped in Angular 10), esm2015, fesm2015), in which case the ngcc converts the package to the Ivy format by changing package.json and the entry files inside node_modules or it is a Ivy distribution (NO umd, fesm2015, esm2020, festm2020), in which case the Angular linker directly consume the package without any changes applied within the package in node_modules.

The Ivy compiler has been the default since version 9 of Angular, so for any consumer using Ivy it should still work, but for consumers using ViewEngine (manually configurable until v12), this would break.

Feel free to reach out to the Angular team, if there is a way to provide both distribution formats, but I'd guess they would recommend just providing the ViewEngine format, since it technically works.

Angular Package Format:

@jasonnutter
Copy link
Contributor

@kyubisation Thanks for that insight. We'll follow up when we have an update or more questions!

@sandrooco
Copy link

Maybe another detail, I just stumbled across the Angular release schedule: https://angular.io/guide/releases#support-policy-and-schedule
Angular itself doesn't support 10 anymore...

@jasonnutter
Copy link
Contributor

Maybe another detail, I just stumbled across the Angular release schedule: https://angular.io/guide/releases#support-policy-and-schedule Angular itself doesn't support 10 anymore...

Correct, and as such, Angular 10 will not be supported in the next major version for MSAL Angular, but will continue to be supported by MSAL Angular v2 (as dropping support would technically be a breaking change).

@jasonnutter
Copy link
Contributor

jasonnutter commented Apr 4, 2022

@kyubisation We have merged and published #4154 (as @azure/msal-angular@2.2.0). Please try that out and let us know if that addresses baseline Angular 13 compatibility. If so, we can then look at incrementally addressing other concerns such as Ivy distribution and the type fixes included in this PR (and I would prefer to have separate PRs for each of those). Thanks!

@jasonnutter
Copy link
Contributor

Closing, please open a new PR to incrementally address issues/features not addressed in the latest version of MSAL Angular.

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.

Angular Ivy distribution MSAL Angular support Angular 13 and RxJS v7
6 participants