Move node clients from `msal-common` to `msal-node` #5788

konstantin-msft · 2023-03-13T15:01:57Z

Move 4 clients used only by msal-node from msal-common to msal-node.
- ClientCredentialClient
- DeviceCodeClient
- OnBehalfOfClient
- UsernamePasswordClient
Move request and part of response docs from msal-common to msal-node.
Update references.

sameerag · 2023-03-13T17:25:49Z

lib/msal-node/src/client/ClientCredentialClient.ts

-import { ServerAuthorizationTokenResponse } from "../response/ServerAuthorizationTokenResponse";
-import { IAppTokenProvider } from "../config/AppTokenProvider";
-import { UrlString } from "../url/UrlString";
+import { ClientConfiguration } from "@azure/msal-common/src/config/ClientConfiguration";


Are we doing package based imports now?

No we should not do this, these things should be exported from msal-common if not already.

That is what I thought. And this is common pattern across the board, to export the files from msal-common and not the pre-compiled package. @konstantin-msft why the change??

+1, I think the reason most of these aren't exported already is because none of the client classes were implemented at the wrapper library level. Having to export all of these common classes may be an argument against factoring the confidential clients out.

My bad. Fixed imports, thank you.

sameerag

Please update the import paths

derisen · 2023-03-13T19:06:45Z

@konstantin-msft think we should probably relocate this doc to msal-node side as well (and maybe rename it to acquire-token while at it?)

ghost · 2023-03-27T15:00:52Z

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.

codecov-commenter · 2023-03-29T00:25:58Z

Codecov Report

❗ No coverage uploaded for pull request base (msal-browser-v3@cfd9821). Click here to learn what that means.
The diff coverage is n/a.

Flag	Coverage Δ
msal-angular	`96.61% <0.00%> (?)`
msal-browser	`84.13% <0.00%> (?)`
msal-common	`85.10% <0.00%> (?)`
msal-node	`80.07% <0.00%> (?)`
msal-node-extensions	`45.81% <0.00%> (?)`
msal-react	`94.82% <0.00%> (?)`
node-token-validation	`87.28% <0.00%> (?)`

konstantin-msft · 2023-03-31T13:33:16Z

@konstantin-msft think we should probably relocate this doc to msal-node side as well (and maybe rename it to acquire-token while at it?)

That's a very good point. Thank you Dogan!
I have moved request and part of response docs to msal-node. I would suggest keeping naming the same to avoid confusion.

konstantin-msft · 2023-03-31T13:38:07Z

Please update the import paths

Done.

tnorling

Looks good. Just as a general comment; we've been making a lot of major bumps of common lately (which is fine!) and it seems like with this change msal-node will be taking on an even heavier dependency on msal-common APIs, which were previously abstracted away from it. We recently discussed our versioning strategy with regards to common and it may be time to revisit that and seriously consider some of the ideas folks had (i.e. pin dependencies to a specific version or stop publishing it as a separate package and bundle it in with the public packages)

jo-arroyo

Approved, with a few formatting comments

jo-arroyo · 2023-03-31T16:25:11Z

lib/msal-node/docs/response.md

@@ -0,0 +1,6 @@
+MSAL will return an `AuthenticationResult.ts` object as a response to all acquire token APIs:


Add heading here?

Good catch, done

jo-arroyo · 2023-03-31T16:28:44Z

lib/msal-node/test/client/ConfidentialClientApplication.spec.ts

@@ -1,19 +1,36 @@
-import { ConfidentialClientApplication } from './../../src/client/ConfidentialClientApplication';
-import { ClientConfiguration, AuthorizationCodeClient, RefreshTokenClient, AuthenticationResult, ClientCredentialClient, OnBehalfOfClient, UsernamePasswordClient } from '@azure/msal-common';
+import {


Combine imports from src with imports below?

jo-arroyo · 2023-03-31T16:51:32Z

lib/msal-node/test/client/PublicClientApplication.spec.ts

-import { PublicClientApplication } from './../../src/client/PublicClientApplication';
-import { Configuration, ILoopbackClient, InteractiveRequest } from './../../src/index';
+import { PublicClientApplication } from '../../src';
+import { Configuration, DeviceCodeClient, ILoopbackClient, InteractiveRequest } from '../../src';


Combine src imports?

jo-arroyo · 2023-03-31T16:51:39Z

lib/msal-node/test/client/PublicClientApplication.spec.ts

 import { ID_TOKEN_CLAIMS, mockAuthenticationResult, TEST_CONSTANTS, TEST_DATA_CLIENT_INFO } from '../utils/TestConstants';
 import {
-    ClientConfiguration, AuthenticationResult, AuthorizationCodeClient, RefreshTokenClient, UsernamePasswordClient,
-    SilentFlowClient, ProtocolMode, Logger, LogLevel, ClientAuthError, AccountInfo, ServerAuthorizationCodeResponse
+    ClientConfiguration,


Same as above

jo-arroyo · 2023-03-31T16:51:45Z

lib/msal-node/test/client/PublicClientApplication.spec.ts

-import { AuthorizationUrlRequest } from "../../src/request/AuthorizationUrlRequest";
-import { UsernamePasswordRequest } from '../../src/request/UsernamePasswordRequest';
-import { SilentFlowRequest } from '../../src/request/SilentFlowRequest';
+import {


Same as above

konstantin-msft · 2023-03-31T17:29:46Z

Looks good. Just as a general comment; we've been making a lot of major bumps of common lately (which is fine!) and it seems like with this change msal-node will be taking on an even heavier dependency on msal-common APIs, which were previously abstracted away from it. We recently discussed our versioning strategy with regards to common and it may be time to revisit that and seriously consider some of the ideas folks had (i.e. pin dependencies to a specific version or stop publishing it as a separate package and bundle it in with the public packages)

I would say with this PR msal-node will be taking on additional request/response/cache on msal-common types rather than APIs. Other than that, I totally agree - pinning deps to a specific version sounds like a good idea to me.

- Fix unit tests.

- Moved/updated docs.

- Beautify imports.

requested changes are already addressed

1. Move 4 clients used only by `msal-node` from `msal-common` to `msal-node`. - ClientCredentialClient - DeviceCodeClient - OnBehalfOfClient - UsernamePasswordClient 2. Move `request` and part of `response` docs from `msal-common` to `msal-node`. 3. Update references.

ghost · 2023-05-03T21:33:13Z

🎉@azure/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

ghost · 2023-05-03T23:28:51Z

🎉@azure/[email protected] has been released which incorporates this pull request.:tada:

Handy links:

konstantin-msft changed the base branch from dev to msal-browser-v3 March 13, 2023 15:02

github-actions bot added msal-common Related to msal-common package msal-node Related to msal-node package labels Mar 13, 2023

konstantin-msft marked this pull request as ready for review March 13, 2023 15:04

konstantin-msft requested review from sameerag, tnorling, hectormmg, peterzenz, jennyf19, jmprieur, bgavrilMS, Robbie-Microsoft and jo-arroyo as code owners March 13, 2023 15:04

sameerag reviewed Mar 13, 2023

View reviewed changes

sameerag previously requested changes Mar 13, 2023

View reviewed changes

ghost added the Needs: Attention 👋 Awaiting response from the MSAL.js team label Mar 27, 2023

konstantin-msft force-pushed the 2441770_migrate_common_to_node branch from 946a743 to c99aa06 Compare March 29, 2023 00:08

ghost removed the Needs: Attention 👋 Awaiting response from the MSAL.js team label Mar 29, 2023

konstantin-msft requested a review from lalimasharda as a code owner March 29, 2023 00:13

konstantin-msft force-pushed the 2441770_migrate_common_to_node branch from 44bcbe3 to a5a5e85 Compare March 30, 2023 21:43

github-actions bot added the documentation Related to documentation. label Mar 31, 2023

konstantin-msft requested a review from sameerag March 31, 2023 13:35

tnorling approved these changes Mar 31, 2023

View reviewed changes

jo-arroyo approved these changes Mar 31, 2023

View reviewed changes

konstantin-msft added 7 commits March 31, 2023 13:48

- Move node clients from msal-common to msal-node.

522d247

- Fix imports.

46ffb34

- Fix unit tests.

Change files

e334836

- Merge latest changes.

2e02cdd

- Beautify imports.

5a74748

- Moved/updated docs.

- Update docs.

0e6bdeb

- Add heading to response.md.

f1eb1ac

- Beautify imports.

konstantin-msft force-pushed the 2441770_migrate_common_to_node branch from c307187 to f1eb1ac Compare March 31, 2023 17:48

- Fix package-lock.json.

ce858af

github-actions bot added the msal-angular Related to @azure/msal-angular package label Mar 31, 2023

konstantin-msft added 2 commits March 31, 2023 14:08

Change files

8f860e1

- Address eslint errors.

a0b7c6f

konstantin-msft enabled auto-merge (squash) March 31, 2023 18:41

konstantin-msft merged commit 8a82b7c into msal-browser-v3 Mar 31, 2023

konstantin-msft deleted the 2441770_migrate_common_to_node branch March 31, 2023 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move node clients from `msal-common` to `msal-node` #5788

Move node clients from `msal-common` to `msal-node` #5788

konstantin-msft commented Mar 13, 2023 •

edited

Loading

sameerag Mar 13, 2023

tnorling Mar 13, 2023

sameerag Mar 13, 2023

hectormmg Mar 13, 2023

konstantin-msft Mar 31, 2023

sameerag left a comment

derisen commented Mar 13, 2023

ghost commented Mar 27, 2023

codecov-commenter commented Mar 29, 2023 •

edited

Loading

konstantin-msft commented Mar 31, 2023

konstantin-msft commented Mar 31, 2023

tnorling left a comment

jo-arroyo left a comment

jo-arroyo Mar 31, 2023

konstantin-msft Mar 31, 2023

jo-arroyo Mar 31, 2023

konstantin-msft Mar 31, 2023

jo-arroyo Mar 31, 2023

konstantin-msft Mar 31, 2023

jo-arroyo Mar 31, 2023

konstantin-msft Mar 31, 2023

jo-arroyo Mar 31, 2023

konstantin-msft Mar 31, 2023

konstantin-msft commented Mar 31, 2023

ghost commented May 3, 2023

ghost commented May 3, 2023

		@@ -0,0 +1,6 @@
		MSAL will return an `AuthenticationResult.ts` object as a response to all acquire token APIs:

Move node clients from msal-common to msal-node #5788

Move node clients from msal-common to msal-node #5788

Conversation

konstantin-msft commented Mar 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sameerag left a comment

Choose a reason for hiding this comment

derisen commented Mar 13, 2023

ghost commented Mar 27, 2023

codecov-commenter commented Mar 29, 2023 • edited Loading

Codecov Report

konstantin-msft commented Mar 31, 2023

konstantin-msft commented Mar 31, 2023

tnorling left a comment

Choose a reason for hiding this comment

jo-arroyo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konstantin-msft commented Mar 31, 2023

ghost commented May 3, 2023

ghost commented May 3, 2023

Move node clients from `msal-common` to `msal-node` #5788

Move node clients from `msal-common` to `msal-node` #5788

konstantin-msft commented Mar 13, 2023 •

edited

Loading

codecov-commenter commented Mar 29, 2023 •

edited

Loading