[Identity] Create identity common package #24445

KarishmaGhiya · 2023-01-12T01:27:04Z

Packages impacted by this PR

@azure/identity
@azure/identity-common
@azure/identity-vscode
@azure/identity-cache-persistence

Issues associated with this PR

Issue - #20429

Describe the problem that is addressed by this PR

The packages @azure/identity-vscode and @azure/identity-cache-persistence were directly referencing the source code of identity-package and this caused a lot of issues.

When we updated the version of @azure/identity from 2.0.x to 2.1.0-beta.x, the dependency of the plugin packages on identity was still on 2.0.x version. We depend on internal interfaces from identity that are directly pulling them from the src folder of identity in these plugin packages.

With the recent version update of identity package to 3.x.x, the plugin packages identity-vscode and identity-cache-persistence started getting built first in the CI. Upgrading the local version of @azure/identity package no longer satisfies the semver dependency for identity on these plugin packages. We receive errors about not being able to find core packages; these errors are coming from the src files of identity that is not yet built and is being pulled in from the src files of the plugin packages.

If the reference to AzurePluginContext in the plugin packages was made from the @azure/identity package instead of directly importing src files of identity, then we probably would not have run into this issue. Typescript thinks that the identity src files are its own package files and of course we don't have these core packages as dependencies in the plugin packages.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Initial Proposed Solution
Pr - #20340

We thought of pulling out the module AzurePluginContext into a separate identity-common folder and simplifying the src imports of all the 2 plugin packages and the identity package. This would also avoid the building of entire identity package into the dist-esm of identity-plugin packages.

Issue with Initial Proposed Solution
We soon ran into cascading effect where we need to move almost all the code of identity (including Msal related code) except the public APIs into the identity-common package. Segregating the tests and the test-utils for such common code is becoming an overhead and adding complications.

Need of the hour
Just to get around the complications currently we have just upgraded the version of the identity dependency in the identity plugin packages to match the version of identity in the main branch. This introduces a tighter coupling of versioning between these plugin packages and the identity package, requiring releasing plugin packages relative to identity version upgrades.

But we have new packages that are still under proposal for localhost-to-cloud like the @azure/identity-spa and @azure/identity-web that follow the same design as the plugin packages where we are directly importing from the src code of identity. We need to come up with a more formal and well-designed solution so that we can make this a standard. We have also been running into similar issues with packages related to services like key vault and storage.

Updated Proposed Solution
With the purpose of sharing code between the identity package and the plugin packages (and more packages in the future), in a way that allows us to have full control of the changes made to the shared code without hidden implications, we propose the following:

Have an @azure/identity-common package which is also released on npm. We can use a different namespace like @azure-internal to reflect to the users that this is not intended for public use. Other packages in the JS community that have a common package released - @azure/msal-common, @azure/communication-common
Have a separate @azure-test/identity-test-utils package to reduce the complications involved in test-utils segregation and make it more organized.

Checklists

Added impacted package name to the issue description
Added a changelog (if necessary)

witemple-msft · 2023-01-12T16:04:08Z

sdk/identity/identity-cache-persistence/package.json

@@ -60,7 +60,8 @@
  "sideEffects": false,
  "dependencies": {
    "@azure/core-auth": "^1.3.0",
-    "@azure/identity": "^2.1.0",
+    "@azure/identity": "^3.1.3",
+    "@azure/identity-common": "1.0.0-beta.1",


Is the plan that we'll ship a stable version of identity-common before we ship a new version of identity that depends on it?

should we just set the common version to 1.0.0 now then? is there a reason to start with beta.1?

We can start with 1.0.0. No reason to keep it beta

xirzec

Very cool to see this coming together! A few various remarks/thoughts

xirzec · 2023-01-12T20:38:45Z

sdk/identity/identity-common/.eslintrc.json

+    "plugins": ["@azure/azure-sdk"],
+    "extends": ["plugin:@azure/azure-sdk/azure-sdk-base"],
+    "rules": {
+      // this package does not have a module in `dist` because the directory does


are these rules accurate? some of the disables feel off

xirzec · 2023-01-12T20:39:06Z

sdk/identity/identity-common/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) 2022 Microsoft


wonder if we need to start bumping the year in these files?

xirzec · 2023-01-12T20:42:20Z

sdk/identity/identity/package.json

@@ -113,6 +113,7 @@
    "@azure/msal-browser": "^2.32.0",
    "@azure/msal-common": "^9.0.0",
    "@azure/msal-node": "^1.14.4",
+    "@azure/identity-common": "1.0.0-beta.1",


can we remove some dependencies from identity now because of what has moved into common?

xirzec · 2023-01-12T20:43:29Z

sdk/identity/identity/src/credentials/clientCertificateCredential.ts

-import { MsalFlow } from "../msal/flows";
-import { credentialLogger } from "../util/logging";
-import { tracingClient } from "../util/tracing";
+import { MsalFlow } from "@azure/identity-common";


we should combine imports like this now that they come from the same place.

xirzec · 2023-01-12T20:46:04Z

sdk/identity/identity/src/index.ts

+  CredentialUnavailableErrorName,
+  AuthenticationRequiredError,
+  AuthenticationRequiredErrorOptions } from "@azure/identity-common";
+
 import { DefaultAzureCredential } from "./credentials/defaultAzureCredential";

 export {


can we do export { /* things */ } from "@azure/identity-common"; instead of the import/export combo?

xirzec · 2023-01-12T20:48:29Z

sdk/identity/identity/tsconfig.json

@@ -9,6 +9,6 @@
      "@azure/identity": ["./src/index"]
    }
  },
-  "include": ["src/**/*", "test/**/*", "samples-dev/**/*.ts"],
+  "include": ["src/**/*", "test/**/*", "samples-dev/**/*.ts", "../identity-common/src/util/logging.ts", "../identity-common/src/msal/nodeFlows/msalNodeCommon.ts", "../identity-common/src/util/tenantIdUtils.ts", "../identity-common/src/msal/flows.ts", "../identity-common/src/msal/utils.ts", "../identity-common/src/errors.ts", "../identity-common/src/client/identityClient.ts", "../identity-common/src/tokenCredentialOptions.ts", "../identity-common/src/regionalAuthority.ts", "../identity-common/src/msal/nodeFlows/tokenCachePersistenceOptions.ts", "../identity-common/src/constants.ts", "../identity-common/test/msalTestUtils.ts", "../identity-common/src/util/processMultiTenantRequest.ts", "../identity-common/src/credentials/multiTenantTokenCredentialOptions.ts", "../identity-common/src/util/processMultiTenantRequest.browser.ts", "../identity-common/src/msal/credentials.ts", "../identity-common/src/msal/types.ts", "../identity-common/src/util/identityTokenEndpoint.ts", "../identity-common/src/util/tracing.ts", "../identity-common/src/credentials/managedIdentityCredential/utils.ts"],


why do we need to include identity-common files here? I thought each of these would be imported from the dependency instead of compiled by TS?

xirzec · 2023-01-12T20:49:22Z

sdk/identity/identity-common/test/msalTestUtils.ts

+
+// Browser tests fail if dotenv.config is called in that environment.
+if (isNode) {
+  dotenv.config({ path: ".env" });


I think v2 of recorder calls dotenv.config automatically?

xirzec · 2023-01-12T20:55:15Z

sdk/identity/identity-common/review/identity-common.api.md

+}
+
+// @public
+export abstract class MsalNode extends MsalBaseUtilities implements MsalFlow {


I think my meta-question for this package is how did we determine what all to hoist into it?

I know we needed to put at least the plugin stuff and related interfaces, and I think I understand that there are some useful common helpers (e.g. formatters, loggers, error shapes) that we want to be consistent across plugins, but were there other goals?

Exposing the MSAL wrapper details will make it a little trickier to refactor those credentials in the future, for example.

That was my main intention as well. I didn't want to pull the MSAL wrapper into the common package. But the tests in the plugin packages were referencing some of these MSAL wrappers for types for creating tests and this is why I had to pull in these wrappers into identity-common. In case the tests could be written without referencing the wrappers then we won't have to do this. cc: @willmtemple

I think we need a different type of abstraction for the testing components. For those it would be sufficient to have a local devDep that we don't publish rather than putting everything into a public package. identity-test or some such.

witemple-msft · 2023-01-12T16:04:59Z

sdk/identity/identity-cache-persistence/test/internal/node/clientCertificateCredential.spec.ts

-} from "../../../../identity/src";
-import { MsalTestCleanup, msalNodeTestSetup } from "../../../../identity/test/msalTestUtils";
+} from "@azure/identity";
+import { MsalTestCleanup, msalNodeTestSetup } from "@azure/identity-common";


Are we shipping this test stuff in the public surface area of identity-common?

ghost · 2023-03-24T10:03:54Z

Hi @KarishmaGhiya. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

ghost · 2023-03-31T11:03:12Z

Hi @KarishmaGhiya. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

KarishmaGhiya added 14 commits January 11, 2023 15:57

create identity-common

0bfdf52

second iteration

42d5249

update identity src links

58b7d19

refactor identity-common

1ccb9f6

update the identity-common

50fc488

update the identity-common

70659fa

update the links

923fec1

update references

14f272e

update package-json

d5277b8

update package-json

20171cd

fixing internal tags

846e727

update exports

11ddce4

update refs

32b2527

update the imports and exports for public interface

4033275

ghost added the Azure.Identity label Jan 12, 2023

KarishmaGhiya requested review from timovv, xirzec, mpodwysocki and witemple-msft January 12, 2023 01:41

witemple-msft reviewed Jan 12, 2023

View reviewed changes

xirzec reviewed Jan 12, 2023

View reviewed changes

witemple-msft reviewed Jan 17, 2023

View reviewed changes

ghost added the no-recent-activity There has been no recent activity on this issue. label Mar 24, 2023

ghost closed this Mar 31, 2023

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Identity] Create identity common package #24445

[Identity] Create identity common package #24445

KarishmaGhiya commented Jan 12, 2023 •

edited

Loading

witemple-msft Jan 12, 2023

KarishmaGhiya Jan 12, 2023

xirzec Jan 12, 2023

KarishmaGhiya Jan 13, 2023

xirzec left a comment

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

xirzec Jan 12, 2023

KarishmaGhiya Jan 12, 2023

witemple-msft Jan 17, 2023

witemple-msft Jan 12, 2023

ghost commented Mar 24, 2023

ghost commented Mar 31, 2023

		@@ -0,0 +1,21 @@
		The MIT License (MIT)

		Copyright (c) 2022 Microsoft

[Identity] Create identity common package #24445

[Identity] Create identity common package #24445

Conversation

KarishmaGhiya commented Jan 12, 2023 • edited Loading

Packages impacted by this PR

Issues associated with this PR

Describe the problem that is addressed by this PR

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Checklists

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

xirzec 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

Choose a reason for hiding this comment

ghost commented Mar 24, 2023

ghost commented Mar 31, 2023

KarishmaGhiya commented Jan 12, 2023 •

edited

Loading