feat(migration-tools): Include Migrator in container code, make it single use, use callbacks instead of taking a full loader #23274
Conversation
…m container author
github-actions
bot
added
area: examples
| @@ -23,23 +20,15 @@ export class InventoryListAppModel implements IInventoryListAppModel, IMigratabl | |||
| // To be used by the consumer of the model to pair with an appropriate view. | |||
| public readonly version = "one"; | |||
|
|
|||
| public constructor( | |||
| public readonly inventoryList: IInventoryList, | |||
| private readonly container: IContainer, | |||
There was a problem hiding this comment.
Since Migrator gets the container back directly from the LoadSourceContainerCallback and can dispose() it directly, the model doesn't need to hold the reference.
|
|
||
| const exportDataCallback = async (container: IContainer): Promise<unknown> => { | ||
| // TODO: verify IMigratableModel | ||
| const exportModel = await getModelFromContainer<IMigratableModel>(container); |
There was a problem hiding this comment.
Since the container code provides the ExportDataCallback itself, the model doesn't need to necessarily conform to any specific interface like IMigratableModel, the container code author can grab the data however they like.
| import type { IContainer } from "@fluidframework/container-definitions/internal"; | ||
| import { | ||
| type ILoaderProps, | ||
| loadExistingContainer, | ||
| } from "@fluidframework/container-loader/internal"; | ||
| import { RouterliciousDocumentServiceFactory } from "@fluidframework/routerlicious-driver/internal"; |
There was a problem hiding this comment.
Can we avoid /internal imports in here? (Examples should not use /internal APIs.)
Perhaps update .eslintrc.cjs to have:
"import/no-internal-modules": [
"error",
{
allow: [
// Within Fluid Framework allow import of '/alpha' from other FF packages.
"@fluidframework/*/alpha",or similar
There was a problem hiding this comment.
Hmm, in general our in-repo examples do use /internal - I can give it a shot for some of these though
There was a problem hiding this comment.
The problem seems to be that I need the full exception list to concatenate (otherwise this simply overrides the existing exception list) and everything blows up. I could duplicate that exception list here but that has its own drawbacks.
Might be good to consider a centralized override for examples I suppose
There was a problem hiding this comment.
I think we should probably have an examples config (aka external to platform config). I don't know how composable configs are. I do know that merging settings isn't supported like we'd want for this rule.
| ), | ||
| codeLoader: new DemoCodeLoader(), | ||
| generateCreateNewRequest: createTinyliciousCreateNewRequest, | ||
| export const setupContainer = async ( |
There was a problem hiding this comment.
nit: prefer named function (export async function setupContainer()
There was a problem hiding this comment.
Why is that? I usually prefer arrow unless I explicitly want this binding.
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
…ngle use, use callbacks instead of taking a full loader (microsoft#23274)
This change centers around moving the Migrator into the container code (whereas today it's expected that the host will bring it and pair it with the migration tool inside the container). This minimizes expectations on the host, who now just needs to provide callbacks to (1) load a copy of the source container for export and (2) do whatever they like with the exported data. With these scoped callbacks, the general result is that both the container code and host get relaxed requirements and more flexibility.
With these scoped callbacks, there doesn't need to be any agreement on loader shape (and so SimpleLoader goes away). They can also be less opinionated on the "future" container, e.g. it'd theoretically be possible to use this even across incompatible breaking changes to the loader layer.
We can also lift the requirement of a specific
IMigratableModel-based implementation, as now it's just the container code author communicating with themselves rather than communicating with a part brought by the host. So now the container code author can use any technique they like for exporting data. TheIMigratableModelinterface is still there (moved to the example package) in this PR but could probably be stripped down a good bit for simplicity.Since Migrator is now inside the container code, it drops its "load the new container automatically" responsibility, which is left instead for the host to do. Migrator is now single-use similar to MigrationTool, bound to the source container. We can also hide MigrationTool completely and just provide the IMigrator to the container code author directly.
This also removes TaskManager usage from MigrationTool. It could still be used to minimize duplication, but it'd impose additional responsibilities on the host to handle abort signals and restarting their flows. Since we need a container deduping strategy anyway, I figured it's OK to remove for now. We can consider adding it back if we find it necessary.
AB#25114