improvement(client): API surface cleanup#26435
improvement(client): API surface cleanup#26435jason-ha wants to merge 2 commits intomicrosoft:mainfrom
Conversation
core-interfaces:
- promote `BrandedType` to `@public`
- rename `InternalUtilityTypes` export to `InternalCoreInterfacesUtilityTypes` so that there are no conflicts with similar namespaces across FF APIs.
- remove unused api-extractor-lint.json config file
- add api-extractor linting for internal/exposedUtilityTypes using workaround that combines that export set with index.js that covers regular export set
- add api-extractor noted forgotten exports and tag some with `@beta`
- correct some `{@link}` references
presence:
- rename exposed `Internal*` namespaces to uniqueness throughout FF APIs
jason-ha
requested review from
Josmithr,
WillieHabi,
alexvy86 and
tylerbutler
There was a problem hiding this comment.
Pull request overview
Cleans up and disambiguates API surface area across Fluid packages by renaming internal utility namespaces, promoting BrandedType visibility, and tightening api-extractor linting coverage (including the internal/exposedUtilityTypes entrypoint).
Changes:
- Renames
InternalUtilityTypes(core-interfaces) toInternalCoreInterfacesUtilityTypesand updates downstream consumers. - Renames presence’s exposed internal namespaces to
InternalPresenceTypes/InternalPresenceUtilityTypesto avoid cross-package name collisions. - Adds api-extractor linting for
internal/exposedUtilityTypesvia a workaround entrypoint; removes an unused api-extractor config.
Reviewed changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/runtime/container-runtime-definitions/src/containerExtension.ts | Updates import/usage to InternalCoreInterfacesUtilityTypes. |
| packages/framework/presence/src/validatableTypes.ts | Updates type utilities usage to InternalCoreInterfacesUtilityTypes. |
| packages/framework/presence/src/test/testUtils.ts | Updates type-level test helper to use renamed utility namespace. |
| packages/framework/presence/src/internalUtils.ts | Updates internal type helper to use renamed utility namespace. |
| packages/framework/presence/src/index.ts | Renames exported internal namespaces to InternalPresenceTypes / InternalPresenceUtilityTypes. |
| packages/framework/presence/src/exposedUtilityTypes.ts | Switches to importing InternalCoreInterfacesUtilityTypes from core-interfaces exposed utility types. |
| packages/framework/presence/src/exposedInternalTypes.ts | Updates {@link} references to new exported namespace names. |
| packages/framework/presence/api-report/presence.legacy.alpha.api.md | Reflects renamed presence internal namespaces in legacy alpha API report. |
| packages/framework/presence/api-report/presence.beta.api.md | Reflects renamed presence internal namespaces in beta API report. |
| packages/framework/presence/api-report/presence.alpha.api.md | Reflects renamed presence internal namespaces in alpha API report. |
| packages/common/core-interfaces/src/test/testValues.ts | Updates tests to import renamed utility namespace via alias. |
| packages/common/core-interfaces/src/test/jsonSerializable.spec.ts | Updates tests to import renamed utility namespace via alias. |
| packages/common/core-interfaces/src/test/jsonDeserialized.spec.ts | Updates tests to import renamed utility namespace via alias. |
| packages/common/core-interfaces/src/shallowReadonly.ts | Tags supported-generics default type as @beta (system) and aligns with re-exports. |
| packages/common/core-interfaces/src/internal.ts | Renames internal exported namespace to InternalCoreInterfacesUtilityTypes. |
| packages/common/core-interfaces/src/exposedUtilityTypes.ts | Expands exposed utility type exports and aliases utility namespace to InternalCoreInterfacesUtilityTypes. |
| packages/common/core-interfaces/src/exposedInternalUtilityTypes.ts | Updates {@link} references related to renamed external-facing alias. |
| packages/common/core-interfaces/src/deepReadonly.ts | Tags supported-generics default type as @beta (system) and aligns with re-exports. |
| packages/common/core-interfaces/src/brandedType.ts | Promotes BrandedType to @public. |
| packages/common/core-interfaces/src/api-extractor/exposedUtilityTypes.ts | Adds workaround entrypoint file for api-extractor lint coverage. |
| packages/common/core-interfaces/package.json | Adds a new api-extractor lint script for esm:internal/exposedUtilityTypes. |
| packages/common/core-interfaces/api-report/core-interfaces.public.api.md | Captures BrandedType as public in the generated API report. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.public.api.md | Captures BrandedType as public in the legacy public API report. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.beta.api.md | Captures BrandedType as public in the legacy beta API report. |
| packages/common/core-interfaces/api-report/core-interfaces.legacy.alpha.api.md | Captures BrandedType as public in the legacy alpha API report. |
| packages/common/core-interfaces/api-report/core-interfaces.beta.api.md | Captures BrandedType as public in the beta API report. |
| packages/common/core-interfaces/api-extractor/api-extractor-lint-internal-exposedUtilityTypes.esm.json | Adds api-extractor lint configuration for the new entrypoint. |
| packages/common/core-interfaces/api-extractor-lint.json | Removes unused api-extractor lint config. |
| * @remarks | ||
| * See {@link InternalTypes.ValueRequiredState}. | ||
| * See {@link InternalPresenceTypes.ValueRequiredState}. | ||
| * |
There was a problem hiding this comment.
InternalPresenceTypes is not a symbol declared in this file (the namespace here is InternalTypes, and the InternalPresenceTypes name only exists as an export alias from index.ts). This {@link ...} is therefore likely to be unresolved/broken for editors and/or api-extractor. Prefer linking to the in-scope symbol (e.g. {@link InternalTypes.ValueRequiredState} or an unqualified {@link ValueRequiredState} within the namespace).
There was a problem hiding this comment.
Unfortunately, limitations of api-extractor require use of the exported name rather than the internally valid name.
| * For known cases, construct a custom interface that extends | ||
| * {@link InternalTypes.ValueStateMetadata}. | ||
| * {@link InternalPresenceTypes.ValueStateMetadata}. | ||
| * |
There was a problem hiding this comment.
InternalPresenceTypes is not declared in this file (it is only an export alias from index.ts), so this {@link ...} is likely unresolved/broken. Prefer linking to the local/in-scope symbol (e.g. {@link InternalTypes.ValueStateMetadata} / {@link ValueStateMetadata}) to keep links valid regardless of export aliasing.
There was a problem hiding this comment.
Unfortunately, limitations of api-extractor require use of the exported name rather than the internally valid name.
| * | ||
| * @remarks | ||
| * In a recursive context, use {@link InternalUtilityTypes.IfExactTypeInTuple} to manage ancestry. | ||
| * In a recursive context, use {@link InternalCoreInterfacesUtilityTypes.IfExactTypeInTuple} to manage ancestry. |
There was a problem hiding this comment.
This {@link InternalCoreInterfacesUtilityTypes.IfExactTypeInTuple} reference appears inside the InternalUtilityTypes namespace, and InternalCoreInterfacesUtilityTypes is not declared in this file. That likely makes the link unresolved/broken. Prefer linking to an in-scope name (e.g. {@link IfExactTypeInTuple} or {@link InternalUtilityTypes.IfExactTypeInTuple}) so the link survives export aliasing.
| * In a recursive context, use {@link InternalCoreInterfacesUtilityTypes.IfExactTypeInTuple} to manage ancestry. | |
| * In a recursive context, use {@link IfExactTypeInTuple} to manage ancestry. |
There was a problem hiding this comment.
Unfortunately, limitations of api-extractor require use of the exported name rather than the internally and/or relatively valid name. (It does mean internal IntelliSense is corrupted.)
|
|
||
| /** | ||
| * Helper for {@link JsonSerializableFilter} to determine if a property may | ||
| * Helper for {@link InternalCoreInterfacesUtilityTypes.JsonSerializableFilter} to determine if a property may |
There was a problem hiding this comment.
InternalCoreInterfacesUtilityTypes is not a symbol declared in this file (the namespace here is InternalUtilityTypes), so this {@link ...} is likely to be unresolved/broken. Prefer linking to the in-scope type (e.g. {@link JsonSerializableFilter} / {@link InternalUtilityTypes.JsonSerializableFilter}) to keep links valid.
| * Helper for {@link InternalCoreInterfacesUtilityTypes.JsonSerializableFilter} to determine if a property may | |
| * Helper for {@link JsonSerializableFilter} to determine if a property may |
There was a problem hiding this comment.
Unfortunately, limitations of api-extractor require use of the exported name rather than the internally and/or relatively valid name. (It does mean internal IntelliSense is corrupted.)
|
|
||
| /** | ||
| * Recurses `T` applying {@link InternalUtilityTypes.JsonDeserializedFilter} up until | ||
| * Recurses `T` applying {@link InternalCoreInterfacesUtilityTypes.JsonDeserializedFilter} up until |
There was a problem hiding this comment.
InternalCoreInterfacesUtilityTypes is not declared in this file, so linking to InternalCoreInterfacesUtilityTypes.JsonDeserializedFilter is likely to break. Prefer {@link JsonDeserializedFilter} / {@link InternalUtilityTypes.JsonDeserializedFilter} (in-scope) to avoid unresolved links when the namespace is exported under an alias.
| * Recurses `T` applying {@link InternalCoreInterfacesUtilityTypes.JsonDeserializedFilter} up until | |
| * Recurses `T` applying {@link JsonDeserializedFilter} up until |
| /** | ||
| * Recurses `T` applying {@link InternalUtilityTypes.DeepReadonlyWorker} up to `RecurseLimit` times. | ||
| * Recurses `T` applying {@link InternalCoreInterfacesUtilityTypes.DeepReadonlyWorker} up to `RecurseLimit` times. | ||
| * |
There was a problem hiding this comment.
InternalCoreInterfacesUtilityTypes is not declared in this file, so this {@link InternalCoreInterfacesUtilityTypes.DeepReadonlyWorker} is likely unresolved/broken. Prefer linking to the in-scope symbol (e.g. {@link DeepReadonlyWorker} / {@link InternalUtilityTypes.DeepReadonlyWorker}) so the reference remains valid regardless of export aliasing.
|
|
||
| /** | ||
| * Core implementation of {@link InternalUtilityTypes.DeepReadonlyLimitingRecursion}. | ||
| * Core implementation of {@link InternalCoreInterfacesUtilityTypes.DeepReadonlyLimitingRecursion}. |
There was a problem hiding this comment.
InternalCoreInterfacesUtilityTypes is not declared in this file (the namespace here is InternalUtilityTypes), so this {@link ...} likely won't resolve. Prefer linking to the in-scope symbol (e.g. {@link DeepReadonlyLimitingRecursion} / {@link InternalUtilityTypes.DeepReadonlyLimitingRecursion}) to keep the doc link valid.
| * Core implementation of {@link InternalCoreInterfacesUtilityTypes.DeepReadonlyLimitingRecursion}. | |
| * Core implementation of {@link InternalUtilityTypes.DeepReadonlyLimitingRecursion}. |
packages/common/core-interfaces/src/api-extractor/exposedUtilityTypes.ts
Show resolved
Hide resolved
So that any forwarding so ensured to find complete and usable TS Docs.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
alexvy86
left a comment
alexvy86
left a comment
There was a problem hiding this comment.
I don't have enough context to comment on whether the change is correct/good, but left a question.
| * ``` | ||
| * | ||
| * @internal | ||
| * @public |
There was a problem hiding this comment.
I'm not sure what implications come from exposing a declare class... e.g. should it @system too (as I would imagine we'd want for an actual class which is just a type utility)? @sealed?
There was a problem hiding this comment.
None of those. This is a type and not an actual class.
It isn't really @system because the use pattern is to have it be a part of non-system types and changes to it could be disruptive.
It isn't @sealed because one good pattern is to inherit another declared class from it.
Its own docs should help note those things - though GitHub diff makes you work to read that.
There was a problem hiding this comment.
FWIW BrandedType has been a part of API surface since #24733. It should have been promoted to @beta then but we had lint hole (fixed in this PR). Then with #24785 it became more prominent. All uses remain details of other types; so, at least from how I like to see a refined surface leaving it @internal was fine.
Moving it to @public rather than @beta as we plan to promote presence to @public which will require this and many other to be @public.
BrandedType in @public is also recommended to fix issue we have with LogLevel.
3 participants
core-interfaces:
BrandedTypeto@publicInternalUtilityTypesexport toInternalCoreInterfacesUtilityTypesso that there are no conflicts with similar namespaces across FF APIs.@beta{@link}referencespresence:
Internal*namespaces to uniqueness throughout FF APIs