Skip to content
This repository has been archived by the owner. It is now read-only.

refactoring #29

Merged
merged 7 commits into from
Mar 13, 2020
Merged

refactoring #29

merged 7 commits into from
Mar 13, 2020

Conversation

derisen
Copy link
Contributor

@derisen derisen commented Mar 9, 2020

CHANGELOG

  • Separated authentication with popup vs. redirect flows into different files.
  • Added fallback check and a backup CDN for importing msal.js
  • Moved loginRequest and tokenRequest objects into authConfig.js
  • Minor fixes on Readme.md

@derisen derisen linked an issue Mar 9, 2020 that may be closed by this pull request
@derisen
Copy link
Contributor Author

derisen commented Mar 9, 2020

@archieag FYI

<!-- polyfilling fetch -->
<script type="text/javascript" src="https://cdn.jsdelivr.net/npm/whatwg-fetch/dist/fetch.umd.min.js"></script>
<!-- depending on your browser you might want to include babel polyfill -->
<script type="text/javascript" src="https://cdn.jsdelivr.net/npm/@babel/polyfill@7.4.4/dist/polyfill.min.js"></script>

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was adding pollyfills for ie11 support but there are a bunch of other things that still don't work and forcing a very restricted js usage (at least without something like npm and babel).

Happily though, I've just been told by a few CxP people that aad samples can drop support for ie11. Removing this part then.

README.md Outdated
@@ -4,7 +4,6 @@ languages:
- javascript
products:
- microsoft-identity-platform
- azure-active-directory-v2

Choose a reason for hiding this comment

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

Still seems relevant, why was it removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had the impression that for new samples microsoft identity platform should replace azure active directory v2 (at least that's what dotnet samples are doing)


// fallback to interaction when silent call fails
return myMSALObj.acquireTokenPopup(request)
.then(tokenResponse => {})
Copy link

@jasonnutter jasonnutter Mar 10, 2020

Choose a reason for hiding this comment

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

This will result in nothing getting returned from getTokenPopup. Either omit this .then, or make sure this chain returns tokenResponse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. kept it there to keep the pattern:

        return myMSALObj.acquireTokenPopup(request)
          .then(tokenResponse => {
            return tokenResponse;
          }).catch(error => {
            console.log(error);
          });

@derisen derisen requested a review from jasonnutter March 10, 2020 22:20
Copy link
Contributor

@pkanher617 pkanher617 left a comment

Choose a reason for hiding this comment

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

This looks fine to me, I was able to run the redirect and popup sample as expected.

@derisen
Copy link
Contributor Author

derisen commented Mar 13, 2020

Thank you very much for all the reviews! Merging this now..

@derisen derisen merged commit 7161a7c into quickstart Mar 13, 2020
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No config.js in project file
3 participants