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

Feature/angular 13 #4467

Closed
wants to merge 8 commits into from
Closed

Feature/angular 13 #4467

wants to merge 8 commits into from

Conversation

sandrooco
Copy link

This PR includes updates for msal-angular.

@ghost
Copy link

ghost commented Feb 1, 2022

CLA assistant check
All CLA requirements met.

@jasonnutter
Copy link
Contributor

@sandrooco Thanks for opening this! We'll take a look and provide feedback.

@codecov-commenter
Copy link

Codecov Report

Merging #4467 (2d6ec57) into dev (76473e9) will decrease coverage by 0.00%.
The diff coverage is n/a.

Flag Coverage Δ *Carryforward flag
msal-angular 96.34% <ø> (-0.05%) ⬇️
msal-browser 87.51% <ø> (ø) Carriedforward from 76473e9
msal-common 85.32% <ø> (ø) Carriedforward from 76473e9
msal-core 82.65% <ø> (ø) Carriedforward from 76473e9
msal-node 82.97% <ø> (ø) Carriedforward from 76473e9
msal-node-extensions 76.03% <ø> (ø) Carriedforward from 76473e9
msal-react 92.70% <ø> (ø) Carriedforward from 76473e9

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
lib/msal-angular/src/msal.guard.ts 90.14% <ø> (ø)
lib/msal-angular/src/msal.interceptor.ts 100.00% <ø> (ø)
lib/msal-angular/src/test.ts

@mills-andrew
Copy link

bumping to show interest! Would love to see this implemented

},
"peerDependencies": {
"@azure/msal-browser": "^2.21.0",
"rxjs": "^6.0.0"
"rxjs": "^7.5.0"
Copy link

@elgurinn elgurinn Feb 11, 2022

Choose a reason for hiding this comment

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

Isn't it possible to support both rxjs 6 and 7 (Same as Angular does currently)?

e.g.

"rxjs": "^6.5.3 || ^7.5.0",

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree, given dropping support for v6 would be a breaking change, which we will not accept.

Copy link
Author

Choose a reason for hiding this comment

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

I updated the rxjs range accordingly to support latest 6.x to 7.x.

Copy link
Contributor

Choose a reason for hiding this comment

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

@sandrooco Apologies for the delayed response, thanks for addressing. To confirm, the updated import statements are compatible with v6 and v7 (this was my biggest concern)?

Copy link
Author

Choose a reason for hiding this comment

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

@jasonnutter Thanks for checking in.
Actually you are right - the new imports are not compatible. (7.1.0 is the last version with the old imports.) If I'm not mistaken msal can live with 7.x and the customer project with 6.x.

Also I honestly don't understand why dropping 6.x support would be a problem. In my opinion, people should upgrade to the latest 7.x version because 7.5 also is the default when setting up Angular 13 projects.
What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

How would this work for older versions of Angular (MSAL Angular v2 still supports Angular 9-12)?

Copy link
Contributor

Choose a reason for hiding this comment

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

i.e. apps that still have rxjs@6 (the point being we can't update to 7 if it would break apps on older versions of rxjs).

@ghost
Copy link

ghost commented Feb 28, 2022

Reminder: The next release is scheduled for next week and this PR appears to be stale :(

If changes have been requested please address feedback.
If this PR is still a work in progress please mark as draft.

@ghost ghost added the Needs: Attention 👋 Awaiting response from the MSAL.js team label Feb 28, 2022
@ghost ghost removed the Needs: Attention 👋 Awaiting response from the MSAL.js team label Mar 1, 2022
@sandrooco
Copy link
Author

@jasonnutter Please review :)

@jasonnutter
Copy link
Contributor

jasonnutter commented Mar 3, 2022

Thanks everyone for their patience, apologies for the delay in getting this update figured out. @Robbie-Microsoft from our team will be working to get this work over the line.

@sandrooco We'll be retargeting this PR against a new branch, both so that we can get the CI/CD fully running and so that we can add a couple other things before we can merge into dev (which @Robbie-Microsoft will be working on). Thanks again!

@jasonnutter jasonnutter linked an issue Mar 3, 2022 that may be closed by this pull request
@sandrooco
Copy link
Author

Thanks everyone for their patience, apologies for the delay in getting this update figured out. @Robbie-Microsoft from our team will be working to get this work over the line.

@sandrooco We'll be retargeting this PR against a new branch, both so that we can get the CI/CD fully running and so that we can add a couple other things before we can merge into dev (which @Robbie-Microsoft will be working on). Thanks again!

Nice, thanks! Should we close this PR then?

@pkwieci1
Copy link

pkwieci1 commented Mar 9, 2022

I'm oh so glad that someone has pushed the ball rolling :)
Thank you @sandrooco!

@mills-andrew
Copy link

any update on this push?

@jasonnutter
Copy link
Contributor

any update on this push?

We're planning to have this addressed by our next release (April 4).

@jasonnutter
Copy link
Contributor

Closing in favor on #4605, which will be released today.

@jasonnutter jasonnutter closed this Apr 4, 2022
@sandrooco sandrooco deleted the feature/angular-13 branch April 5, 2022 07:04
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.

MSAL Angular support Angular 13 and RxJS v7
8 participants