-
Notifications
You must be signed in to change notification settings - Fork 556
Stub out proposed non-legacy root tree compatible container and service APIs, including local service #25422
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ce APIs, including local service
import { createContainer } from "@fluidframework/runtime-definitions/internal"; | ||
|
||
describe("treeDataStore", () => { | ||
it("example use", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is an example use of the proposed APIs, showing collab without needing any mocks, internal or even legacy APIs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things I like in particular:
- Local service-client is a gap we should fill
- Starting to introduce code-loading concepts to the service-client in the form of the "registry"
- The container simply returns a single data object rather than the initialObjects thing
I'd particularly suggest:
- Explore/prototype the implementation of
createContainer
andservice.attachContainer()
, at least enough to make sure you don't hit any walls with the way theLoader
andContainer
are built today. - Try integrating into one of our examples (rather than just unit tests), probably external-controller would be a good choice. IMO these are helpful to get diversity of usage patterns, and will help inform whether this is a generalizable pattern.
import type { | ||
ServiceOptions, | ||
ServiceClient, | ||
} from "@fluidframework/runtime-definitions/internal"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At an abstract level, seems wrong for the driver layer to have a dependency on the runtime layer
import { pkgVersion } from "./packageVersion.js"; | ||
|
||
/** | ||
* Creates and returns a document service for local use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment says this will return a document service (makes sense, we're in the driver), but function name implies it will create a service client (doesn't make sense, service clients - at least using today's definition - are a composition of the driver + loader layer).
Not exactly sure what this will do since there's not an implementation to refer to.
export interface DataStoreKind<T> extends ErasedType<readonly ["DataStoreFactory", T]> {} | ||
|
||
/** | ||
* A connection to a Fluid storage service. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems wrong, or maybe you're redefining what a service client is? The interface below still looks fairly similar to the preexisting concept of a service client though.
* | ||
* @privateRemarks | ||
* This will likely end up needing many of IFluidContainer's APIs, like disconnect, connectionState, events etc. | ||
* Before adding them though, care should be taken to consider if they can be improved or simplified. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree with this mentality, though I'd warn that IFluidContainer exists for this exact same reason. We wanted to be cautious/selective about bringing IContainer's baggage over to IFluidContainer (and thankfully, it's at least not 100% of the baggage) but just know that while I'm supportive, it's an uphill battle that historically hasn't been funded.
Maybe consider if there's a way to avoid having "a third thing" that is separate from everything else, maybe a interface IFluidContainer extends FluidContainerButWeThinkTheseThingsAreGood
or something? Or in general just think about how we can avoid making the problem grow if we don't achieve/fund this goal of simplifying IFluidContainer's warts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For APIs that are common, we would want one top extend a subset of the other (or share a common base). For now though, I'm not bothering as the API is so small, and making that work would require some extra plumbing (I think the existing types would need to pull their members from this new one due to the layering)
* @param id - The unique identifier of the container to load. | ||
* @param root - The {@link DataStoreKind} for the root, or a registry which will be used to look up the root based on its type. | ||
* | ||
* @throws a UsageError if the DataStoreKind's type (either the root directly or looked up from the registry) does not match the type of the root data store in the container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exception seems maybe a bit rough - if the container isn't loaded yet, then presumably there's no way to inspect the container and determine the correct kind.
Your privateRemarks below allude to what's basically code loading - I'd probably suggest exploring that direction further and make it mandatory even (don't accept a raw DataStoreKind since there's no way to know you have the right kind a priori to loading). "Registry" is probably a loaded term here, I'd also suggest aligning with code loading terminology over registry terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you only care about opening one particular kind of container (for example your applications container format, or the kind used in an example), I think its ergonomic for that common case to not force the construction of a registry. I suspect this will be the common case.
If the BrainStorm app tries to open a Whiteboard document for example, it's not going to dynamic code load anything, its just going to error. We don't need to make BrainStorm support dynamic code loading based on root type if all but one case is just going to error anyway. Same goes for most apps: its common (though not the only case) for an app to try and open a file rather than for an OS to attempt to locate the correct application by inspecting a file based on its contents (not extension).
Fluid cares a lot about simple examples being able to actually be really simple/minimal looking in the public API surface. As this will appear in lots of examples, allowing the example code to be that much cleaner, while not losing and functionality for users who want it seems like a win.
I think this API as is does a good job at both allowing the simple case in a nice strong typed easy to use way (but erroring if the app ever encounters incompatible data) while providing you the ability to do your own non exception based error handling if you want.
That said, it is simpler implementation and API wise to only take Registries, it's just that such simplicity has been a much lower priority than clean looking minimal examples when I have worked on fluid public APIs in the past.
* | ||
* @alpha | ||
*/ | ||
export function createEphemeralServiceClient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Ephemeral" is a loaded term (currently used to mean AFR containers created without persisted storage), suggest picking a different term (maybe just stick with "local"?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have one existing supported public service implementation, AzureLocalService. I want to differentiate from that so calling it LocalService would be a bit confusing (Somehow it's what you get by subtracting the azureness?). In AzureLocalService, the local is meaning "not in the Microsoft cloud", but can persist across sessions and connect to multiple processes.
Also, we explicitly want to imply that there is no persisted storage, so using the same term AFR uses to imply the same thing might not be a downside here.
Thinking about how these names relate to existing browser storage APIs, AzureLocalService is kind of like local storage, we don't have anything like session storage (but its possible), and this is even more local, so maybe we should call it "inMemory" service, or "InMemoeyEphemeralService"?
|
||
const container1 = await service.attachContainer(createContainer(myFactory)); | ||
|
||
assert.equal(container1.root.root, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aesthetically .root.root
doesn't feel great - does "root" make sense for the FluidContainer member? If FluidContainer isn't itself aware of a deeper nesting structure it's a little odd for it to refer to it as the root, so maybe this would be better as container1.data.root
or something?
|
||
const service = createEphemeralServiceClient(); | ||
|
||
const container1 = await service.attachContainer(createContainer(myFactory)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under the covers, creating a Container
even before attach requires providing the service up-front (probably it shouldn't, but that's how it is today). I'm curious what the implementation of createContainer(myFactory)
might look like or if this might be somewhat of a blocker - the usage here looks like it's trying to be service-agnostic until attach happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under the covers, creating a Container even before attach requires providing the service up-front
That was one of my fears. Good to have that confirmed.
probably it shouldn't, but that's how it is today
My thought as well.
I think we should aim to fix that (someday), but for now I'll provide a temporary workaround for it since fixing that seems like a longer term project.
* | ||
* The returned promise resolves once the container is attached: the container from the promise is the same one passed in as the argument. | ||
*/ | ||
attachContainer<T>(detached: FluidContainer<T>): Promise<FluidContainerAttached<T>>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you intend to support service-specific functionality with this pattern (PR description alludes to eventually supporting other services)? E.g. the Audience
members are uniquely typed for each service (e.g. see AzureContainerServices
). Creating containers is a little different between the services too (e.g. see AzureRemoteConnectionConfig
vs. OdspConnectionConfig
, which ultimately feed into the create container request). If so, where do you expect to surface those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't realize we had a pattern of having service specific data accessible from the container.
Good to know that's a thing: I'll tweak the design to accommodate it.
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output
|
Description
This add a simple API as a cleaner alternative to fluid-static and aquaduct, initially targeting just unit tests with local service with good support for SharedTree (but not specific to it.).
The intention is to address the use case of simple tree collab examples, API tests and customer app testing without relying on the legacy API surface or internal test APIs.
Once this use case is addressed by these alpha APIs, broader use cases would be considered, including support for other services and actual interactive applications. Such cases would be considered before these APIs would be promoted to beta or public, but for now they are not a priority.
That said, this is intended as a clean replacement for aquaduct and fluid-static eventually, so some consideration is given to those API surfaces. Below are some know issues with them that this new API surface should address:
Currently our non-legacy (fluid-static declarative model) API surface has a few problems:
Additionally, the legacy APIs also have some issues:
Reviewer Guidance
The review process is outlined on this wiki page.
This is an early draft of a proposed API. It is only ready for some basic discussion.