Create service specific FluidStatic instances #5815

sumedhb1995 · 2021-04-14T21:47:16Z

The goal of this PR is to allow each service to create its own independent FluidStatic implementation, so that they can each uniquely handle the creation & loading of containers based on the configuration requirements the service needs.

Changes introduced here and the rationale for them are:

The IGetContainerService now provides requires callbacks filled out for createContainer and getContainer. This allows the service to provide their own unique implementation instead of forcing them all down the same path. These callbacks take in a new fileConfig parameter which is unique to each service and is responsible for holding the metadata for the actual file backing the Fluid container. For Tiny/Routerlicious, this is the id, whereas for ODSP, this would hold the driveId, siteUrl, and folderName. An example of the OdspService can be found here
Update TinyliciousService and RouterliciousService to implement the new interface
Replace the singular FluidStatic with FluidStaticTinylicious and FluidStaticRouterlicious. These two are fairly similar, apart from Tinylicious taking in a port number, whereas Routerlicious takes in a config for its init. However, it sets the stage for different services to provide their own FluidStatic implementations, which will have different method signatures for the init, createContainer, and getContainer calls. An example of FluidStaticOdsp can be found here

I've kept these changes together as the reason for updating the IGetContainerServices is so that we can use them to have unique FluidStatic implementations for each service, and this shows how the updated interface would be consumed.

skylerjokiel

Overall I like the direction of this. I've left a bunch of high level direction comments that I want to use this PR to bottom out on.

experimental/framework/fluid-static/src/FluidStaticRouterlicious.ts

skylerjokiel · 2021-04-15T01:20:55Z

experimental/framework/fluid-static/src/FluidStaticTinylicious.ts

+ * Singular global instance that lets the developer define all Container interactions with the Tinylicious service
+ */
+let globalFluidTinylicious: FluidTinyliciousInstance | undefined;
+export const FluidTinylicious = {


Let's give thought naming here from both the package and object perspective. We should have a convention across the Services we provide. For packages I want to propose a 1:1 mapping between the service and the import and avoid grouping multiple services in a single package. Even if they share the same driver or have the same api.

package naming

Tinylicious will need both the client and server bits. I did a bit of research and these are the general paradigms.

I'm leaning towards #4.

tinylicious and have both client and server in the package

tinylicious-client and tinylicious

tinylicious-client and tinylicious-server

1 & 3 - support both where tinylicious is a re-export of the other two for simplicity

import naming

For the naming nothing is really jumping out. I think I like #2 but am open to more ideas

import { FluidTinylicious } from "tinylicious-client";

import Tinylicious from "tinylicious-client";

import { Fluid } from "tinylicious-client";

let's rename get-container to match what we choose above.

I'm working on creating a draft to show how FluidStaticTinylicious and TinyliciousService can be taken out of their existing locations in fluid-static and get-container respectively to be moved to a separate "tinylicious-client" package
However, we may want to stage those changes in two parts, the first where we add in the elements to the existing packages and then where we take them out into its own individual package

I've created the draft PR with the changes to separate out tinylicious-client into its own package here: #5838
It doesn't remove get-container entirely but takes out tinyliciousService from it and move it into the new tinylicious-client package, which also hosts FluidTinylicious.
I was looking into re-exporting out the actual Tinylicious BE server package itself from the same package as well but, as it stands, that package doesn't provide a export yet so I believe that would need to be added first followed by the re-export

skylerjokiel · 2021-04-15T01:34:24Z

experimental/framework/get-container/src/getContainer.ts

@@ -15,6 +13,14 @@ import {
 export interface IGetContainerService {


I don't think this interface is needed if we consolidate packages.

skylerjokiel · 2021-04-15T01:32:25Z

experimental/framework/fluid-static/src/FluidStaticTinylicious.ts

+/**
+ * FluidTinyliciousInstance provides the ability to have a Fluid object backed by a Tinylicious service
+ */
+export class FluidTinyliciousInstance {


I don't think we need two packages here. It should just be one client package for the tinylicious client bits named how we choose. We can then consolidate the logic.

Consolidated logic into @fluid-experimental/tinylicious-client package here #5838

skylerjokiel · 2021-04-15T01:36:15Z

experimental/framework/get-container/src/tinyliciousService.ts

 import { getContainer, IGetContainerService } from "./getContainer";

+export interface ITinyliciousFileConfig {


I really don't like File here.

So the idea is that we have two configs. Once defines the service specific container properties. The other defines the container itself.

Maybe serviceConfig and objectConfig ? or serviceConfig and schemaConfig?

Also we might want to separate create and get? maybe... That would mess with the generic though...

Updated to be serviceConfig & objectConfig.
As for separation of create & get, OdspService actually expects two different interfaces for each respectively, where the create takes additional file metadata such as the its drive info and folder name but the get requires the URL to the file. It just so happens that the serviceConfig for Tinylicious is the same for both cases

skylerjokiel · 2021-04-15T01:38:51Z

examples/hosts/app-integration/external-controller/src/app.ts

-import Fluid from "@fluid-experimental/fluid-static";
-import { TinyliciousService } from "@fluid-experimental/get-container";
+import { FluidTinylicious } from "@fluid-experimental/fluid-static";
+import { ITinyliciousFileConfig } from "@fluid-experimental/get-container";


This should be coming from the same package as the Tinylicious client bits. Let's consolidate the two packages. More comments below.

external-controller only requires an import from @fluid-experimental/tinylicious-client in the follow-up #5838

skylerjokiel · 2021-04-15T01:43:48Z

examples/hosts/app-integration/external-controller/src/app.ts

 import { SharedMap } from "@fluidframework/map";
 import { DiceRollerController } from "./controller";
 import { renderDiceRoller } from "./view";

 // Define the server we will be using and initialize Fluid
-const service = new TinyliciousService();
-Fluid.init(service);
+FluidTinylicious.init();


If we keep the init I see three configs.

Service specific properties that apply across the application (ex. FRS endpoint, tokens)

Container specific properties that are required for the service (ex. ID)

Container specific properties that define the data schema

Correct. There is an initial config to set up the service itself. Then there are two more for the container, one which is service-specific and the other defining the objects within the container

…to create_get_services

sumedhb1995 · 2021-04-16T21:29:43Z

@skylerjokiel I've updated this PR to take in the recommended changes to FluidTinylicious and the config naming. All of the changes here are still in the existing packages of get-container and fluid-static.

I agree with the idea of moving the service-specific logic that is spread across those two packages into one. To address that, I've created this follow-up draft PR to show how we could create a "tinylicious-client" package, with default export FluidTinylicious, allowing developers to only require importing from one package per service https://github.com/microsoft/FluidFramework/pull/5838/files

Since creating the new packages results in a number of other changes to set the new package up, update dependencies, etc., I've kept the two separate but please let me know if you'd like me to merge it in here

skylerjokiel · 2021-04-21T03:19:03Z

I think #5383 replaces this PR. If so can you close this.

sumedhb1995 · 2021-04-21T16:57:07Z

Closing this as we have moved to implementing separate client packages for each service with #5383

Sumedh Bhattacharya added 4 commits April 14, 2021 10:28

move create/get to services

0832c55

create service specific tinylicious fluid static

bd14d98

add static routerlicious

0632e24

maintain service specific logic

89089f3

sumedhb1995 requested review from micahgodbolt, ChumpChief, skylerjokiel, SamBroner, nmsimons and danielroney April 14, 2021 21:47

policy check

156ae1d

skylerjokiel suggested changes Apr 15, 2021

View reviewed changes

Sumedh Bhattacharya added 2 commits April 15, 2021 13:10

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

1d6922f

…to create_get_services

update config naming, drop routerlicious

e32dd78

github-actions bot requested a review from skylerjokiel April 15, 2021 23:50

sumedhb1995 mentioned this pull request Apr 16, 2021

Separate out Tinylicious client into its own package #5838

Merged

Sumedh Bhattacharya added 2 commits April 16, 2021 13:45

Merge branch 'main' of https://github.com/microsoft/FluidFramework in…

34eb481

…to create_get_services

update changelog

782ceb1

sumedhb1995 closed this Apr 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create service specific FluidStatic instances #5815

Create service specific FluidStatic instances #5815

sumedhb1995 commented Apr 14, 2021

skylerjokiel left a comment

skylerjokiel Apr 15, 2021

skylerjokiel Apr 15, 2021

sumedhb1995 Apr 15, 2021

sumedhb1995 Apr 16, 2021

skylerjokiel Apr 15, 2021

skylerjokiel Apr 15, 2021

sumedhb1995 Apr 16, 2021

skylerjokiel Apr 15, 2021

sumedhb1995 Apr 15, 2021

skylerjokiel Apr 15, 2021

sumedhb1995 Apr 16, 2021

skylerjokiel Apr 15, 2021

sumedhb1995 Apr 15, 2021

sumedhb1995 commented Apr 16, 2021 •

edited

Loading

skylerjokiel commented Apr 21, 2021

sumedhb1995 commented Apr 21, 2021

		@@ -15,6 +13,14 @@ import {
		export interface IGetContainerService {

		import { getContainer, IGetContainerService } from "./getContainer";

		export interface ITinyliciousFileConfig {

Create service specific FluidStatic instances #5815

Create service specific FluidStatic instances #5815

Conversation

sumedhb1995 commented Apr 14, 2021

skylerjokiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

package naming

import naming

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sumedhb1995 commented Apr 16, 2021 • edited Loading

skylerjokiel commented Apr 21, 2021

sumedhb1995 commented Apr 21, 2021

sumedhb1995 commented Apr 16, 2021 •

edited

Loading