FieldKind cleanup #25869
FieldKind cleanup #25869
Conversation
github-actions
bot
added
base: main
| }); | ||
|
|
||
| export const valueFieldEditor: ValueFieldEditor = { | ||
| export const requiredFieldEditor: RequiredFieldEditor = { |
There was a problem hiding this comment.
rename from "value" to "required" was done long ago, but not everything was renamed. The field kind identifier has to stay, but these kinds of things can be updated to make things more consistant.
| * @param identifier - Globally scoped identifier. | ||
| * @param multiplicity - bound on the number of children that fields of this kind may have. | ||
| * TODO: replace with numeric upper and lower bounds. |
There was a problem hiding this comment.
These docs were moved to FieldKindData, and still apply here (and show up in IntelliSense) due to implements FieldKindData. As these are internal, IntelliSense is what matters most.
| * @param changeHandler - Change handling policy. | ||
| * @param allowsTreeSupersetOf - returns true iff `superset` supports all that this does | ||
| * and `superset` is an allowed upgrade. Does not have to handle the `never` cases. | ||
| * See {@link isNeverField}. | ||
| * TODO: when used as a method (instead of a free function like the other superset related functions), | ||
| * this name is/signature is confusing and seems backwards. | ||
| * @param handlesEditsFrom - Kinds (in addition to this) whose edits can be processed by changeHandler. | ||
| * If the kind of a field changes, and edits are rebased across that kind change, | ||
| * listing the other old kind here can prevent those edits from being conflicted and | ||
| * provide a chance to handle them. |
There was a problem hiding this comment.
These docs moved to the members of FieldKindOptions
| * provide a chance to handle them. | ||
| */ | ||
| // TODO: provide this and use it for improved support for rebasing changes across schema upgrades. | ||
| // readonly handlesEditsFrom: ReadonlySet<FieldKindIdentifier>; |
There was a problem hiding this comment.
We never implemented the ability to preserve edits across schema changes, so this option which only exists to allow preserving edits to a field whose schema has changed never has had an opportunity to do anything, and has also never been provided as anything but empty.
Someday we will likely want this back, but for now it does not need to exist.
| * @remarks | ||
| * Specifies everything needed to determine if a given field schema upgrade is allowed. | ||
| */ | ||
| export interface FieldKindOptions<TFieldChangeHandler> { |
There was a problem hiding this comment.
This was made force some of the less obvious from context parameters to the constructor into named members of an object for improved readability.
packages/dds/tree/src/feature-libraries/default-schema/defaultFieldKinds.ts
Outdated
Show resolved
Hide resolved
| * Compatibility portion of a {@link FlexFieldKind}. | ||
| * | ||
| * @remarks | ||
| * Specifies everything needed to determine if a given field schema upgrade is allowed. | ||
| */ | ||
| export interface FieldKindOptions<TFieldChangeHandler> { |
There was a problem hiding this comment.
This seems a bit misleading to me. The change handler is responsible for a lot more than just determining compatibility and handling schema upgrades. FieldKindOptions contains basically all of the field kind's policy. I would change the doc comment to reflect your PR comment, that this is just a bag of named arguments for field kind construction.
Co-authored-by: alex-pardes <[email protected]>
2 participants
Description
Refactors to cleanup FlexFieldKind and its implementations.
Reviewer Guidance
The review process is outlined on this wiki page.