tree: Promote schemaCreationUtilities to beta #20035
Conversation
github-actions
bot
added
area: dds
| @@ -61,32 +66,49 @@ export function singletonSchema<TScope extends string, TName extends string | nu | |||
| * Converts an enum into a collection of schema which can be used in a union. | |||
| * @remarks | |||
| * Currently only supports `string` enums. | |||
| * The string value of the enum is used as the name of the schema: ensure that its stable and unique. | |||
There was a problem hiding this comment.
Nit:
| * The string value of the enum is used as the name of the schema: ensure that its stable and unique. | |
| * The string value of the enum is used as the name of the schema: you must ensure that its stable and unique. |
There was a problem hiding this comment.
I don't think there is a clear "you" here (ex: neither of us as reviewers or implementers are the "you"). Perhaps "callers" would make more sense?
| export function adaptEnum< | ||
| TScope extends string, | ||
| const TEnum extends Record<string, string | number>, | ||
| >(factory: SchemaFactory<TScope>, members: TEnum) { |
There was a problem hiding this comment.
Can we add an explicit return type here? That is required by our all of our non-minimal (now deprecated) eslint configs.
There was a problem hiding this comment.
Omitting it here was intentional. Specifying the return the here is messy since it can't refer to any of the typers in the function. Additionally doing so causes an implicit conversion to this messy hard to maintain type, which I feel is both less type safe, and annoying. For reference the return type here would be
(<TValue extends TEnum[keyof TEnum]>(value: TValue) => TreeNode & {
readonly value: TValue;
}) & { readonly [Property in keyof TEnum]: TreeNodeSchemaClass<ScopedSchemaName<TScope, TEnum[Property]>, NodeKind.Object, TreeNode & {
readonly value: TEnum[Property];
}, never, true, unknown> & (new () => TreeNode & {
readonly value: TEnum[Property];
}); };Given that anyone using this from outside the package will see that due to using the d.ts files, and anyone working on the package can see it in the API report, d.ts files, public docs and intelisense, I just don't see much value from including it in the .ts file explicitly when the current approach lets us express that type as typeof factoryOut & TOut; which is way simpler.
Generally I prefer explicit return types, but when they are a huge mess like this, I think they are more harm than good. I'd rather let the compiler come up with it, then review that (ex: in this PR, where I got that from the api report)
That said, I still see value it the argument for explicit return types: if you do really want them here I can do that, but I think it makes this code less nice of an example of how you can relatively simply write utilities that combine schema as it becomes about equal parts runtime logic and types (which duplicate the same thing).
There was a problem hiding this comment.
Left a couple of mechanical comments.
The API (based on the example you added) is more complex than seems ideal - mostly, the required call to typedObjectValues when using the union in a schema. Would it be possible to refactor things such that this step isn't required?
Also interested in @nmsimons's thoughts on the API.
Co-authored-by: Joshua Smithrud <54606601+Josmithr@users.noreply.github.com>
I want the semantics defined here: https://github.com/microsoft/FluidFramework/wiki/Release-Tags#beta (specifically I want feedback from customers, but reserve the ability to change the API). I do not want the semantics defined for alpha ( https://github.com/microsoft/FluidFramework/wiki/Release-Tags#alpha ) as these APIs do not meet those requirements at all. They are not "currently in use by known partners (with previous agreement), but which we do not plan to support long term." Its the exact opposite of that. Therefore I think alpha would be very misleading to use here and beta has the exact documented semantics I want. If getting an API to the level of quality where we are allowed to ask for feedback from customers via beta means we need to do some extra work, we can work toward that. Marking them as alpha instead might not require as much quality review, but I don't think alpha is a reasonable shortcut as it sends the exact wrong message (that these are legacy and being removed, have existing users, and don't want feedback). |
|
This is still valid, just waiting for FF 2.1 is its feature work not required for 2.0 |
Description
Promote schemaCreationUtilities to beta, and a bit of polish for their API and docs.
This is not targeting FF 2.0: it can get stabilized later (if ever).
Reviewer Guidance
The review process is outlined on this wiki page.