Skip to content

[msal-node-extensions]: Add electron webpack sample - #4034

Merged
Samuel Kubai (samuelkubai) merged 17 commits into
devfrom
samples/electron-webpack-msal-node-extensions
Sep 27, 2021
Merged

[msal-node-extensions]: Add electron webpack sample#4034
Samuel Kubai (samuelkubai) merged 17 commits into
devfrom
samples/electron-webpack-msal-node-extensions

Conversation

@samuelkubai

@samuelkubai Samuel Kubai (samuelkubai) commented Sep 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor

This pull request aims to fix the webpack compatibility issue by doing the following:-

  • Fork the node-bindings library with the following changes
  • Implement the API changes in node-extensions
  • Add the electron webpack sample for msal-node-extensions

Note:
The created fork can be found here

@github-actions github-actions Bot added documentation Related to documentation. extensions Related to extensions for the base libraries msal-common Related to msal-common package labels Sep 6, 2021
@codecov-commenter

Codecov Comments Bot (codecov-commenter) commented Sep 6, 2021 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.24%. Comparing base (1324cee) to head (1c1852c).
⚠️ Report is 3892 commits behind head on dev.

Additional details and impacted files
Flag Coverage Δ
msal-angular 96.31% <ø> (ø)
msal-browser 88.48% <ø> (∅)
msal-common 85.10% <ø> (ø)
msal-core 76.90% <ø> (ø)
msal-node 81.45% <ø> (ø)
msal-react 94.17% <ø> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasonnutter

Jason Nutter (jasonnutter) commented Sep 7, 2021

Copy link
Copy Markdown
Contributor

Can you share the changes made in your fork of bindings?

@samuelkubai

Samuel Kubai (samuelkubai) commented Sep 9, 2021

Copy link
Copy Markdown
Contributor Author

Can you share the changes made in your fork of bindings?

Sure thing, I want to move the changes to a fork under AzureAD, so I can update the pull request.

@github-actions github-actions Bot added msal@1.x and removed msal-common Related to msal-common package labels Sep 9, 2021
@github-actions github-actions Bot added msal-angular Related to @azure/msal-angular package msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package msal-react Related to @azure/msal-react labels Sep 9, 2021
@samuelkubai
Samuel Kubai (samuelkubai) marked this pull request as ready for review September 9, 2021 17:14
jasonnutter
Jason Nutter (jasonnutter) reviewed Sep 13, 2021
View reviewed changes
Comment thread lib/msal-core/package.json Outdated
jasonnutter
Jason Nutter (jasonnutter) reviewed Sep 13, 2021
View reviewed changes
Comment thread extensions/samples/electron-webpack/package.json Outdated
derisen
Doğan Erişen (derisen) reviewed Sep 13, 2021
View reviewed changes
Comment thread extensions/samples/electron-webpack/README.md
derisen
Doğan Erişen (derisen) reviewed Sep 13, 2021
View reviewed changes
Comment thread extensions/samples/electron-webpack/src/main.ts
derisen
Doğan Erişen (derisen) reviewed Sep 13, 2021
View reviewed changes
Comment thread extensions/samples/electron-webpack/src/main.ts
derisen
Doğan Erişen (derisen) reviewed Sep 13, 2021
View reviewed changes
Comment thread extensions/msal-node-extensions/package.json Outdated
jasonnutter
Jason Nutter (jasonnutter) approved these changes Sep 20, 2021
View reviewed changes

@jasonnutter Jason Nutter (jasonnutter) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good! We should setup some time to talk about how we want to approach your fork of bindings long-term, but this is good for now.

derisen
Doğan Erişen (derisen) reviewed Sep 20, 2021
View reviewed changes
Comment thread extensions/samples/electron-webpack/README.md Outdated
derisen
Doğan Erişen (derisen) approved these changes Sep 20, 2021
View reviewed changes

@derisen Doğan Erişen (derisen) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM! Apologies for delay, getting node-gyp to work is painful.. Left a minor comment.

sameerag
Sameera Gajjarapu (sameerag) reviewed Sep 27, 2021
View reviewed changes
Comment thread extensions/msal-node-extensions/package.json
"dependencies": {
"@azure/msal-common": "^5.0.0",
"bindings": "^1.5.0",
"bindings": "git://github.com/samuelkubai/node-bindings.git#v1.6.0",

@sameerag Sameera Gajjarapu (sameerag) Sep 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Did we follow up on creating an azure repo for this?

@samuelkubai Samuel Kubai (samuelkubai) Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes we did, we realized it would take a lot longer to process and thus decided to go on with this setup for the time being as we research on how best to switch to a repo under AzureAD

sameerag
Sameera Gajjarapu (sameerag) approved these changes Sep 27, 2021
View reviewed changes

@sameerag Sameera Gajjarapu (sameerag) left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved. I have a concern merging this - please ensure that your fork is covered under open source licensing so we can proceed with the merge.

@samuelkubai
Samuel Kubai (samuelkubai) deleted the samples/electron-webpack-msal-node-extensions branch September 27, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sameerag Sameera Gajjarapu (sameerag) sameerag approved these changes
@hectormmg Hector Morales (hectormmg) Awaiting requested review from hectormmg
@jo-arroyo Jo Arroyo (jo-arroyo) Awaiting requested review from jo-arroyo
@pkanher617 Prithvi Kanherkar (pkanher617) Awaiting requested review from pkanher617
@tnorling Thomas Norling (tnorling) Awaiting requested review from tnorling
+2 more reviewers
@jasonnutter Jason Nutter (jasonnutter) jasonnutter approved these changes
@derisen Doğan Erişen (derisen) derisen approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

documentation Related to documentation. extensions Related to extensions for the base libraries msal-angular Related to @azure/msal-angular package msal-browser Related to msal-browser package msal-common Related to msal-common package msal-node Related to msal-node package msal-react Related to @azure/msal-react

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@samuelkubai @codecov-commenter @jasonnutter @derisen @sameerag