feat(odsp-client): Add etag file conversion API attach options #25804
Conversation
github-actions
bot
added
public api change
|
🔗 Found some broken links! 💔 Run a link check locally to find them. See linkcheck output |
|
|
||
| /** | ||
| * Optional eTag to use when attaching the container. | ||
| * If provided, the container will |
There was a problem hiding this comment.
Incomplete sentence here, FYI
There was a problem hiding this comment.
Nit: any chance we could add interface level docs here? 😁
There was a problem hiding this comment.
Since this type is explicitly used as input, we should probably tag it as @input.
https://github.com/microsoft/FluidFramework/wiki/TSDoc-Guidelines#input
There was a problem hiding this comment.
Also, since this is strictly used as input, it probably makes sense to make all of the properties readonly.
| * The ID of the item (file) to which the container is being attached. | ||
| * When combined with eTag, this will trigger a conversion of an existing file to a Fluid file. | ||
| */ | ||
| itemId?: string; |
There was a problem hiding this comment.
Nit: we should probably type these with | undefined to match the others. With our current TypeScript configurations, it doesn't make a big difference. But longer term we want to enable exactOptionalPropertyTypes, at which point the distinction matters (and these should all probably align).
|
|
||
| /** | ||
| * The ID of the item (file) to which the container is being attached. | ||
| * When combined with eTag, this will trigger a conversion of an existing file to a Fluid file. |
There was a problem hiding this comment.
How coupled are itemId and eTag? Would it make sense to combine them into a single object property? Or are they really independent?
There was a problem hiding this comment.
The feature only works with both, so I can couple them; however, their the same "level" of metadata as the other properties here, so it would be a bit weird to nest them I think
There was a problem hiding this comment.
Oh, do these correspond to external ODSP properties or something? If so, should we document that here?
There was a problem hiding this comment.
Yeah this is information from the external (not-fluid) ODSP file
There was a problem hiding this comment.
Are there public docs we can point to from the interface-level comment? Might be nice to make that relationship explicit in the docs.
2 participants
Description
Portion of #25597 to expose Attach options that utilize an API feature from SPO to convert an existing file to Fluid format. All new options are optional.
Reviewer Guidance
New params are optional, and existing functionality is not altered. Builds off of #25000 for additional context.