Export StagingMode as legacy/alpha #25377
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a legacy/alpha entrypoint to runtime-definitions and exports staging mode as legacy/alpha instead of legacy/beta. It also fixes an issue in the build tools where node10 compatibility files weren't properly handling relative paths in nested subdirectories.
Key changes:
- Moves staging mode APIs from legacy/beta to legacy/alpha classification
- Adds a new legacy/alpha entrypoint to runtime-definitions package
- Fixes build tool path resolution for nested subdirectory entrypoints
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| patches/@fluid-tools__build-cli@0.58.1.patch | Adds patch for build CLI to fix relative path handling in nested directories |
| packages/runtime/runtime-definitions/src/dataStoreContext.ts | Changes staging mode APIs from @legacy @beta to @legacy @Alpha |
| packages/runtime/runtime-definitions/package.json | Adds legacy/alpha entrypoint and splits build scripts for both legacy levels |
| packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.beta.api.md | Removes staging mode APIs from legacy beta API report |
| packages/runtime/datastore/src/dataStoreRuntime.ts | Removes eslint-disable comments for deprecated imports |
| packages/runtime/container-runtime/src/containerRuntime.ts | Removes eslint-disable comments for deprecated imports |
| package.json | Adds the new build CLI patch to patchedDependencies |
| build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts | Fixes path resolution for nested directory entrypoints using relative paths |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
jason-ha
left a comment
There was a problem hiding this comment.
Patches are not great for toolchain validation. We can release build tools updates pretty easily; so I recommend using normal flow.
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.beta.api.md
Show resolved
Hide resolved
build-tools/packages/build-cli/src/library/commands/generateEntrypoints.ts
Outdated
Show resolved
Hide resolved
| # API report changes | ||
| /**/api-report/*.public.api.md @microsoft/fluid-cr-api | ||
| /**/api-report/*.beta.api.md @microsoft/fluid-cr-api | ||
| /**/api-report/*.legacy.*.api.md @microsoft/fluid-cr-api |
There was a problem hiding this comment.
this just included legacy alpha, which is not needed now, as the public and beta rule above will appropriately cover legacy for public and beta, and legacy alpha no longer needs api review
packages/runtime/runtime-definitions/api-report/runtime-definitions.legacy.alpha.api.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
…y-murphy/FluidFramework-1 into runtime-defs-legacy-alpha
…to runtime-defs-legacy-alpha
| * @privateRemarks After partners move to the alpha interfaces this interface should be renamed and tagged to be internal. | ||
| */ | ||
| export interface StageControlsExperimental { |
There was a problem hiding this comment.
Seems like there is a strong relationship with StageControlsAlpha. When this goes internal, should it extend StageControlsAlpha?
Otherwise, probably should comment the overlap.
Same comment for similar other cases.
There was a problem hiding this comment.
yeah. i plan to inherit after we release, and partners move to alpha. i don't want to disrupt active prototyping
| containerRuntime as IContainerRuntimeBaseExperimental | ||
| ).enterStagingMode?.(); | ||
| containerRuntime as unknown as ContainerRuntimeBaseAlpha | ||
| ).enterStagingMode(); |
There was a problem hiding this comment.
This looks worse. No better options?
We should probably comment most as use outside of type guards. We should definitely comment cast through unknown.
There was a problem hiding this comment.
keeping it optional kept the cast a bit nicer, but made usage awkward. i think i prefer the harder cast, and the easier usage. we don't have many examples in the codebase for this type of pattern.
There was a problem hiding this comment.
Recommend using a type guard. Failing that an assert.
as is worse than ? IMO and as unknown as is generally terrible (prone to failure).
It is also good to have test code that is a good example of actual use and I don't think recommended pattern is as unknown as.
There was a problem hiding this comment.
yeah. i'm not sure what the right pattern here is yet. i'd like to get this in, so i can see how it works
There was a problem hiding this comment.
2c--when adding to some existing object type, exposing a typeguard also as part of the alpha API is pretty reasonable and gives customers a "safe" (at least as safe as we make it) way to use the APIs:
/**
* @alpha @legacy
*/
export function asContainerRuntimeAlpha(runtime: IContainerRuntime): IContainerRuntimeAlpha {
// whatever validation logic you want can replace
return runtime as unknown as IContainerRuntimeAlpha;
}…to runtime-defs-legacy-alpha
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
|
I'd like this to make the release. since this is the alpha space, we can evolve the usage patterns as we figure them out |
| "$schema": "https://developer.microsoft.com/json-schemas/api-extractor/v7/api-extractor.schema.json", | ||
| "extends": "<projectFolder>/../../../common/build/build-common/api-extractor-report.esm.legacy.json" | ||
| "extends": "<projectFolder>/../../../common/build/build-common/api-extractor-report.esm.legacy.json", | ||
| "mainEntryPointFilePath": "<projectFolder>/lib/legacyAlpha.d.ts", |
There was a problem hiding this comment.
this changes as legacyAlpha.d.ts is the bottom most type def, from which all alpha legacies flow
| "api-extractor:commonjs": "flub generate entrypoints --outFileLegacyBeta legacy --outFileLegacyAlpha legacyAlpha --outDir ./dist", | ||
| "api-extractor:esnext": "flub generate entrypoints --outFileLegacyBeta legacy --outFileLegacyAlpha legacyAlpha --outDir ./lib --node10TypeCompat", |
There was a problem hiding this comment.
The clean script needs updated to remove all generated files. I think just legacy/alpha.d.ts needs cleaned up now.
Adds a legacy/alpha entrypoint to runtime-definitions, and exports staging mode as legacy/alpha. --------- Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
Adds a legacy/alpha entrypoint to runtime-definitions, and exports staging mode as legacy/alpha.