[Identity] Precise Typechecking #31870
Conversation
deyaaeldeen
requested review from
a team,
glharper,
minhanh-phan,
KarishmaGhiya,
maorleger and
schaabs
as code owners
deyaaeldeen
force-pushed
the
identity/precise-typechecking
branch
from
d8317ee
to
d5b377c
Compare
|
API change check API changes are not detected in this pull request. |
deyaaeldeen
force-pushed
the
identity/precise-typechecking
branch
from
d5b377c
to
fc328fb
Compare
deyaaeldeen
force-pushed
the
identity/precise-typechecking
branch
from
fc328fb
to
624653d
Compare
| import { DeveloperSignOnClientId } from "../../../dist/esm/constants.js"; | ||
| import { IdentityClient } from "../../../dist/esm/client/identityClient.js"; |
There was a problem hiding this comment.
This is a public test and I wonder why are these imported from internal code.
There was a problem hiding this comment.
I wasn't aware that we're not able to - so that's a little surprising to me. With that said, identity has been around fro a while and drifts in a few ways from the rest of the packages I suppose. With that said, just like everywhere else we should be importing from src not dist
| import { ClientSecretCredential } from "../../../src/credentials/clientSecretCredential.js"; | ||
| import { IdentityClient } from "../../../src/client/identityClient.js"; | ||
| import { ClientSecretCredential } from "../../../dist/esm/credentials/clientSecretCredential.js"; | ||
| import { IdentityClient } from "../../../dist/esm/client/identityClient.js"; |
There was a problem hiding this comment.
@KarishmaGhiya This is a public test and I wonder why are these imported from internal code.
|
|
||
| return false; | ||
| } | ||
| export * from "./cred.js"; |
There was a problem hiding this comment.
can you help me understand why this change is needed? What was wrong with the existing index file?
| import type { AuthenticationError } from "../src/index.js"; | ||
| import { AzureAuthorityHosts } from "../src/index.js"; | ||
| import type { AuthenticationError } from "@azure/identity"; | ||
| import { AzureAuthorityHosts } from "@azure/identity"; |
| import type { AccessToken, GetTokenOptions, TokenCredential } from "../../src/index.js"; | ||
| import type { CredentialLogger } from "../../src/util/logging.js"; | ||
| import type { AccessToken, GetTokenOptions, TokenCredential } from "@azure/identity"; | ||
| import type { CredentialLogger } from "../../dist/esm/util/logging.js"; |
There was a problem hiding this comment.
something doesn't seem right - we should not be importing from "../../dist" i don think? Same for every import from dist
| import { DeveloperSignOnClientId } from "../../../dist/esm/constants.js"; | ||
| import { IdentityClient } from "../../../dist/esm/client/identityClient.js"; |
There was a problem hiding this comment.
I wasn't aware that we're not able to - so that's a little surprising to me. With that said, identity has been around fro a while and drifts in a few ways from the rest of the packages I suppose. With that said, just like everywhere else we should be importing from src not dist
Follows the same recipe in #31786, more specifically:
@azure/identityin public tests and the compiled esm in internal testsLive run: https://dev.azure.com/azure-sdk/internal/_build/results?buildId=4360514&view=results