feat(Loader): Add create/load Container APIs that give you an object before doing full initialization #23709
Conversation
github-actions
bot
added
area: loader
| * Not to be called directly on this type. | ||
| */ | ||
| public async initialize(): Promise<void> { | ||
| assert(false, "Method will be provided based on create/load API that is used"); |
There was a problem hiding this comment.
The assert text is hard to parse - it becomes clearer what it means only by reading the rest of the code.
I think we can avoid the need to have it, and the need to expose container in weird states.
How about the following:
- create() -> ContainerShell, can register on events only and has only one method:
- ContainerShell.start() -> Container
This clearly requires proxying event registrations from shell to real container underneath.
But this kind of structure completely eliminates the risk of calling some public APIs on Container while it's not initialized. It also completely separates the workflows, allowing to remove APIs from Container (like initialize(), maybe others) that should not be there.
There was a problem hiding this comment.
Problem I faced with that is, do you also keep the shell around and continue to use it for events? It will actually be the same instance as the container returned by start.
| createProps: IContainerCreateProps, | ||
| codeDetails: IFluidCodeDetails, | ||
| ): Promise<Container> { | ||
| ): Container & { initialize: () => Promise<void> } { |
There was a problem hiding this comment.
Container type already has initialize on it - why do you need this new type (that looks identical to Container)?
There was a problem hiding this comment.
Yeah, lots to clean up in implementation, looking for feedback on the API first
There was a problem hiding this comment.
i find api easiest to review in the api reports, so you should update/generate those
| }, | ||
| { start: true, end: true, cancel: "generic" }, | ||
| ); | ||
| return Object.assign(container, { |
There was a problem hiding this comment.
If I got it right, you want symmetrical flow, i.e. call initialize() in both cases.
I'm not sure I like it - while symmetry is nice, it exposes container to users in a new state, which means we now need to test all the things that user can do with such container in this state (before initialize() is called), even if the message is - do not call any API. I'd prefer the flow I described above, and leave detached flow as is, i.e. single call returns detached container.
| createDetachedContainerProps: ICreateDetachedContainerProps, | ||
| ): IContainer & { initialize: () => Promise<void> } { | ||
| const loader = new Loader(createDetachedContainerProps); | ||
| const container = loader.createDetachedContainerUninitialized( |
There was a problem hiding this comment.
I'd find a way not to use Loader in these new workflows. I.e. refactor code enough where loader is not present in this flow (calling APIs that came out of loader is Ok).
Reason being - I'd rather do all the required refactoring right now, potentially taking some risk introducing bugs into this new workflow right now, not later. They will be easier to discover, as clients would be changing their code, and thus putting more scrutiny to test it at that moment. If we do behind the scenes refactoring later, we have higher chance of introducing and not noticing bugs.
That said, the assumption I'm making here - that we will simplify this new workflow, removing (not calling) some functionality on this new path (thus making it more risky). Maybe not a good example (as I think we already disabled it), but if Container caching was still On, I'd expect this new path to stop caching container, while leaving caching on legacy flow. Same for any other functionality that we see in Loader flow that we do not want to see going forward.
| return container.initialize().then(() => container); | ||
| } | ||
|
|
||
| public createDetachedContainerUninitialized( |
There was a problem hiding this comment.
we should be moving away from the loader, so i wouldn't expect any changes in the loader, the functions should do all the work
| { start: true, end: true, cancel: "generic" }, | ||
| ); | ||
|
|
||
| return Object.assign(container, { initialize }); |
There was a problem hiding this comment.
i really don't like the use of object.assign in this pr. ideally we can decompose whatever is happening into free functions and small objects to build up the container, rather than tacking new functions on top the Container monolith
There was a problem hiding this comment.
Yeah I don't either, just playing around. I struggle to communicate what I'm looking for feedback on sometimes with these exploratory PRs.
Description
When using the existing Container load API, which is async, it's possible to receive a closed Container with no way to know what error closed it (the intention is to throw the error if it closes during load, but it's possible that step is missed due to async model).
This PR demonstrates an alternative API which returns a Container object with a bonus async
initializefunction that must be called before the Container is used. This allows the caller to set up listeners for the "closed" event before doing the actual load step.Reviewer Guidance
This is currently a sketch to show the API for design discussion.