feat(core-interfaces): add FluidReadonlyArray type#27747
Conversation
github-actions
Bot
added
area: framework
|
Hi! Thank you for opening this PR. Want me to review it? Based on the diff (690 lines, 12 files), I've queued these reviewers:
How this works
|
2583321 to
5cdf764
Compare
jenn-le
changed the title
7e19418 to
2c8b5d2
Compare
Add FluidReadonlyArray<T> as a TypeScript-version-independent equivalent of ReadonlyArray, following the pattern of FluidReadonlyMap and FluidMap. Includes stable methods through ES2023 (at, findLast, findLastIndex) but excludes copy-on-write methods (toReversed, toSorted, toSpliced, with) that our implementations do not yet provide. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
2c8b5d2 to
49a0424
Compare
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
Bundle size comparisonBase commit: Notable changesNo bundles changed by ≥ 500 bytes parsed. Per-bundle deltas
|
| isAssignableTo<readonly string[], FluidReadonlyArray<string>> | ||
| >; | ||
|
|
||
| // FluidReadonlyArray is NOT assignable to ReadonlyArray due to minor structural differences |
There was a problem hiding this comment.
This makes adopting this type a breaking chain which could be quite problematic for users to adopt.
I think we need to fix that.
There was a problem hiding this comment.
I think the main requirement for this change is that we make this true: as long as its not passing, we don't have a good way to validate that we got our interface correct, and it will break customers which uses our arrays as readonly arrays (which used to be supported) even with typescript configurations where that worked before.
What we want to get out of this change is to keep such users working, but make the case where they change their tsconfig and start getting errors to report the errors in their code and not ours.
| >; | ||
|
|
||
| // FluidReadonlyArray is NOT assignable to ReadonlyArray due to minor structural differences | ||
| // in Symbol.unscopables typing. This is acceptable — the important direction is that |
There was a problem hiding this comment.
Can you put the sentence breaks not in the middle of lines, it makes commenting on sentences hard.
|
|
||
| // FluidReadonlyArray is NOT assignable to ReadonlyArray due to minor structural differences | ||
| // in Symbol.unscopables typing. This is acceptable — the important direction is that | ||
| // native arrays (which ARE ReadonlyArray) remain assignable to FluidReadonlyArray under ES2023+. |
There was a problem hiding this comment.
This is something we specifically do not support users doing, so having it work is probably more harmful then useful. Our type is sealed documenting that this is no allowed: we want this type to be one which only we are allowed to implement it so we can add things to it as needed, such as new API s that get added in 2-35 or whatever which would break code assigning 2023 arrays to our sealed type.
|
|
||
| // Native mutable Array is NOT assignable to FluidReadonlyArray under ES2020 | ||
| // (missing at/findLast/findLastIndex). | ||
| declare type _array_to_fluidReadonlyArray = requireFalse< |
There was a problem hiding this comment.
This, and the above non-assignability checks are not very useful. There are tons of reasons types might not be assignable ( if we implemented our array type as something totally wrong, like number those tests would all still pass, and give 0 useful information).
Additionally this test really tells us pretty much nothing, and also will break when we update to a newer es standard, which we plan to do soon. A test which doesn't validate anything we care about, and will have to be modified in the future has negative value.
|
|
||
| /** | ||
| * Compile-time type utilities for manual type tests in this package. | ||
| * These mirror the pattern used in the auto-generated type test files. |
There was a problem hiding this comment.
Can't this package just import those the same way the generated type tests do?
| * Licensed under the MIT License. | ||
| */ | ||
|
|
||
| import type { FluidIterableIterator, FluidReadonlyArray } from "../../index.js"; |
There was a problem hiding this comment.
Looks like these are the only test changes here: you should also update the tests in import-testing which are skipped due to the issue you are fixing.
Description
Add
FluidReadonlyArray<T>to@fluidframework/core-interfacesas a TypeScript-version-independent equivalent of the built-inReadonlyArraytype, following the same pattern as the existingFluidReadonlyMapandFluidMaptypes.This includes stable methods through ES2023 (
at(),findLast(),findLastIndex()) but deliberately excludes newer copy-on-write methods (toReversed(),toSorted(),toSpliced(),with()) that our implementations do not yet provide. This ensures Fluid Framework types remain safe to implement without being broken by TypeScript updates that add new members toReadonlyArray.The type is introduced at
@betaand re-exported fromfluid-framework. It will be used in a follow-up PR to replace theextends ReadonlyArray<T>inReadonlyArrayNode/TreeArrayNode.Reviewer Guidance
Reviewer guidance wiki
FluidReadonlyArrayinterface is inpackages/common/core-interfaces/src/fluidArray.tssrc/test/types/typeTestUtils.ts(also used by existingfluidMapTypes.ts)