(odsp-client): Add odsp-client as experimental package #18199
Conversation
github-actions
bot
added
area: dev experience
github-actions
bot
removed
the
area: dev experience
sonalideshpandemsft
changed the title
sonalideshpandemsft
force-pushed
the
alpha-m365-client
branch
from
1c36ce3
to
ab7b618
Compare
| OdspMember, | ||
| } from "./interfaces"; | ||
| export { OdspClient } from "./OdspClient"; | ||
| export { OdspAudience } from "./OdspAudience"; |
There was a problem hiding this comment.
Do we need to export the class, or is just the interface sufficient? Do external users need to construct these?
| */ | ||
| export interface OdspConnectionConfig { | ||
| /** | ||
| * Site url representing ODSP resource location |
There was a problem hiding this comment.
Are there more concrete docs we can point to here so users know how to provide this?
| import { ITokenProvider } from '@fluidframework/azure-client'; | ||
| import { ServiceAudience } from '@fluidframework/fluid-static'; | ||
|
|
||
| // @alpha (undocumented) |
There was a problem hiding this comment.
I know this is still a draft, but ideally any of the items currently marked as undocumented should be documented before we merge. More details on these items may also help reviewers evaluate the API surface.
sonalideshpandemsft
force-pushed
the
alpha-m365-client
branch
2 times, most recently
from
387fc76
to
02e5de7
Compare
sonalideshpandemsft
force-pushed
the
alpha-m365-client
branch
3 times, most recently
from
d5641a8
to
c825326
Compare
layerInfo.json
Outdated
| "Runtime", | ||
| "Routerlicious-Driver", | ||
| "Server-Shared-Utils", | ||
| "AzureClient" |
There was a problem hiding this comment.
Ultimately OdspClient probably shouldn't depend on AzureClient (maybe the token stuff moves somewhere else), but this is probably fine for now.
There was a problem hiding this comment.
adding to doc feedback section. will iterate on this in upcoming PRs
| project: ["./tsconfig.json"], | ||
| }, | ||
| rules: { | ||
| "@typescript-eslint/strict-boolean-expressions": "off", |
There was a problem hiding this comment.
Would be great to get this reenabled, but not prereq for initial checkin if it's too much work.
| } | ||
|
|
||
| public async getContainer( | ||
| sharingUrl: string, |
There was a problem hiding this comment.
We can start here, but would be nice if we can make this just an "id" rather than a full URL (this is what AzureClient does, and it's a lot friendlier to hide URL formats away from the customer).
There was a problem hiding this comment.
I've left comments in document in PR about it:
- I'm not sure it has to be sharing link (and types of sharing links we support). It has perf impact.
- I do not understand how it all works if URL points to different drive ID or different siteURI / path that are specified when OdspClient is created. Do we ignore them? What driveID is returned on different APIs - one that was supplied to OdspClient, or one that identifies a file?
- I think we are confusing users about exposing too many concepts, like URI, containerID, itemID. I think we should expose one, and provide static functions to get all the other data.
- I assume we will not allow users to provide their own URI resolvers, and leave them to do conversion of their app API -> our ID/path outside of these interactions (but it would be worth confirming and describing it somewhere)
There was a problem hiding this comment.
Regarding (2):
I do not understand how it all works if URL points to different drive ID or different siteURI / path that are specified when OdspClient is created. Do we ignore them? - I'll look into this more.
What driveID is returned on different APIs - one that was supplied to OdspClient, or one that identifies a file? - Does this mean there will be two separate drive ids? Why would there be a different driveId to identify a file?
You can only use one driveId for any given Fluid Container. So, if you need to use more, you can create additional odsp-clients. But the Drive ID will be different for each tenant.
There was a problem hiding this comment.
@ChumpChief @vladsud I checked the way azure client is doing this. It seems like that id is not enough there too. The azure endpoint is taken in constructor and then using this id we make some modifications to that endpoint and append this id to reach to the request url which is then used to load the container. I am not sure I like that a lot because that is kind of functionality provided by resolver and its utility apis. Similarly, ODSP also has endpoint encoding all this info but imo it is not a sound way to arrive at the request url.
I don't see any harm in taking the url when app can make use of the utility api to construct it and just pass it to this function. And it does not necessarily be a sharing link, getAbsoluteUrl in OdspDriverUrlResoover does not give back a asharing link, but it is still enough to load the container as the absolute url is resolvable by the same resolver.
| const getAttributes = async (): Promise<OdspServiceAttributes> => { | ||
| const resolvedUrl = container.resolvedUrl as IOdspResolvedUrl; | ||
| if (resolvedUrl === undefined) { | ||
| throw new Error("Resolved Url not available on attached container"); |
There was a problem hiding this comment.
In general, we always expect these to be defined after attach. This approach is fine to start with, but it would probably make sense for us to instead await attachment, if the container is not yet attached (rather than reject the promise).
There was a problem hiding this comment.
adding this to doc feedback section
sonalideshpandemsft
force-pushed
the
alpha-m365-client
branch
from
52126d0
to
788eff3
Compare
sonalideshpandemsft
force-pushed
the
alpha-m365-client
branch
from
788eff3
to
5ff9af8
Compare
| @@ -157,6 +157,10 @@ | |||
| "Server-Shared-Utils" | |||
| ] | |||
| }, | |||
| "OdspClient": { | |||
| "dirs": ["packages/service-client/odsp-client"], | |||
| "deps": ["AzureClient"] | |||
There was a problem hiding this comment.
Why does odsp-client need to depend on azure packages? Do we need a new Service-Clients layer for shared stuff between the service clients?
There was a problem hiding this comment.
ITokenResponse is part of azure client. I have opened a work item to move this to fluid static or somewhere else. But that's only the shared piece as of now.
| import { OdspClient } from "@fluid-experimental/odsp-client"; | ||
|
|
||
| const odspClient = new OdspClient(props); | ||
| const { container, services } = await OdspClient.getContainer("_unique-id_", schema); |
There was a problem hiding this comment.
Is this example assuming that top-level await is available?
|
|
||
| ## Using initial objects | ||
|
|
||
| The most common way to use Fluid is through initial collaborative objects that are created when the Container is created. Distributed data structures and DataObjects are both supported types of collaborative objects. |
There was a problem hiding this comment.
Are we ready to talk about data objects? This would be the first time I think.
| }; | ||
|
|
||
| // Fetch back the container that had been created earlier with the same url and schema | ||
| const { container, services } = await OdspClient.getContainer("_unique-url_", schema); |
There was a problem hiding this comment.
Is this a problem specific to odsp-client or a general design problem with the initial objects model?
| }>; | ||
| } | ||
|
|
||
| // @alpha (undocumented) |
| } | ||
|
|
||
| private createLoader(containerSchema: ContainerSchema): Loader { | ||
| const runtimeFactory = new DOProviderContainerRuntimeFactory(containerSchema); |
There was a problem hiding this comment.
What do you mean "file content" defines schema?
| permission: [], | ||
| scopes: [], | ||
| user: { id: "" }, | ||
| mode: "write", |
There was a problem hiding this comment.
If it starts in read mode how does the developer switch?
…rosoft#18305) adds a test for the concurrent restoration and edit of removed tree --------- Co-authored-by: Noah Encke <Noah.Encke@microsoft.com>
…e/relayService (microsoft#18306) ## Description Make improvements in calculating retry after time for calls to storage/relayService. The logic at service side is not very sound to calculate this value. So at least in short term, improving this logic at client side to be intelligent in deciding this value. Loop faced a bug related to this, so we need to provide a short-term viable sol. --------- Co-authored-by: Jatin Garg <jatingarg@Jatins-MacBook-Pro-2.local>
## Description Two recent PRs conflicted to break the build. This fixes it.
## Description Fix import to remove cycle.
github-actions
bot
added
area: dds
github-actions
bot
removed
area: server
|
I'm merging this PR to move forward. I've documented the majority of the feedback as work items in ADO. |
Description
This PR introduces a new client that provides a set of common utility functions to be used whenever developers want to create and load Fluid containers backed by files stored on ODSP. The scope of the current package is marked fluid-experimental and the apis are marked with the alpha release tag.
Reviewer Guidance
Would appreciate feedback on:
Other information or known dependencies
MSFT Internal Design Doc
AB#6224