Exposed package "path" on IComponentContext. #1220

agarwal-navin · 2020-02-10T21:39:11Z

This change adds the package "path" to IComponentContext. This will let the component know the context it was in when it is created. Currently only the container runtime knows this. Initially this will be used just for telemetry.
There is is a small window where a remoted component context is created but is not set until getInitialSnapshotDetails is called. We want to at least log an error in this case.
Also fixes Update createComponent in externalComponentLoader.ts to use the current component's path instead of WaterParkLoaderName #1208 by updating createComponent to use the "path" in IComponentContext instead of hardcoded WaterParkLoaderName as the path.

packages/runtime/container-runtime/src/componentContext.ts

packages/runtime/runtime-definitions/src/components.ts

skylerjokiel

anthony-murphy · 2020-02-11T20:18:36Z

packages/runtime/container-runtime/src/componentContext.ts

@@ -67,6 +67,12 @@ export abstract class ComponentContext extends EventEmitter implements IComponen
        return this._hostRuntime.id;
    }

+    public get packagePath(): string[] | never {


can we make this readonly string[], that way consumers will know it shouldn't be modified

packages/runtime/container-runtime/src/componentContext.ts

anthony-murphy

kurtb · 2020-02-11T20:20:25Z

packages/runtime/runtime-definitions/src/components.ts

+    /**
+     * The package path of the component as per the package factory.
+     */
+    readonly packagePath: string[] | never;


This is separate from this review but why is the path an array? It seems like it should just be a string. And ideally to fit in with the rest of the system a URL.

It is a string array and not just a string because we allowed special character in our types. If we made it a string we would have to not allow / which would have broken backwards compatibility.

kurtb · 2020-02-11T20:27:35Z

packages/runtime/container-runtime/src/componentContext.ts

@@ -67,6 +67,12 @@ export abstract class ComponentContext extends EventEmitter implements IComponen
        return this._hostRuntime.id;
    }

+    public get packagePath(): string[] | never {


At some level what's the point of knowing this...? Doesn't the component already know what it is?

See the other comment - but if we don't want to restructure that way - it would be nice to just have this have a string return type rather than even showing the never. The runtime should just assert and guarantee no client of the API could even call this property prior to use.

The runtime knows this but the component itself doesn't. Since we don't expose it to the Component it doesn't know the context it was in when it was created originally.

It should not be possible for a component to be able to access the context to get the packagePath before it is fully loaded and realized. I added an assert in the packagePath method to make sure no one does this. Since it can assert and not return a value, a return type of never should be fine here right?

For use in subsequent loads I think I see the need to know - I do wonder if we can pass a component handle (or component) that is the factory that loaded you. And so we never actually expose the path directly. Only via a component.

Never might work - but I always find composite types a bit confusing. Especially since if we expose this on the IComponentContext interface I think its best to have the return type be a string[].

The fact that it does existing undefined is more due to how we structure the load. But we should be able to protect against this invariant and so should be able to guarantee that once a client gets access to this it's always a string[].

I agree with you that by exposing string[] we are exposing a design implementation but without a change to our attach message I'm not convinced that this will work.

We've done the work to hide subComponent from component creation which is a great step towards hiding how component registry works. I see exposing this as a way to give developers power for advanced scenarios. The embeddedComponentLoader for example, knows how to dynamically load components at it's included registry. The ECL doesn't know the registry path to itself but it needs to attach based off its registry. It currently assumes that it's just at the root which is not always true.

Without a huge refactor Handles don't really make sense. This is because we are not getting a handle to a Component Instance but to the registry. The registry doesn't necessarily have a resolution path.

kurtb · 2020-02-11T20:31:53Z

packages/runtime/container-runtime/src/componentContext.ts

@@ -426,7 +437,7 @@ export class RemotedComponentContext extends ComponentContext {
        runtime: ContainerRuntime,
        storage: IDocumentStorageService,
        scope: IComponent,
-        private readonly pkg?: string[],
+        pkg?: string[],


This being sometimes optional makes me think we can restructure things to make it always required. I imagine it's because we include the type of component in its summary folder. But use the existence of a path in the tree to know it exists. I'd think the load flow could be updated so that we defer create the actual context.

Yea, I had the same concern. The reason it's not always defined is because of the loading from snapshot scenario. When we load from snapshot we generate a RemoteComponentContext for each component in the snapshot. But the component snapshot body can be a Snapshot blob or a string which is a pointer to a previous snapshot. If it's a string we don't have the pkg type and to get it we would need to resolve the snapshot id. Since we don't know if this component will be used we don't want to make and resolve needless things.

It is optional here but it should not be possible for a component to have access to the context before it is realized at which point the pkg should be available.

To be clear Kurt, I think you're right that we can delay context creation till we try to resolve the componentRuntime. I'm just not sure it's even worth it. I also think it's a bigger change than this. The componentContext is designed to be a proxy object given to the componentRuntime. So the runtime should really be the only thing interacting with the context.

Yeah - the fix to make this non-optional might not be worth the cost - since you'd have to restructure how we have the local vs. remote contexts.

But I think as far as a client of the runtime goes - they'll never see a context where this isn't defined - and we can probably put in asserts to guard against this.

kurtb · 2020-02-11T20:33:32Z

packages/runtime/container-runtime/src/componentContext.ts

@@ -384,6 +391,10 @@ export abstract class ComponentContext extends EventEmitter implements IComponen
        this.loaded = true;
        this.componentRuntime = componentRuntime;

+        // Freeze the package path to ensure that someone doesn't modify it when it is
+        // returned in packagePath().
+        Object.freeze(this.pkg);


If pkg can be a string it avoids needing to freeze as well.

If you're concerned you could also just clone the array rather than requiring the freeze which we don't use very often in the rest of the code.

SamBroner · 2020-02-11T21:44:54Z

I can't tell what the goal of this is... is this effectively replacing a component registry? i.e. are you intending to use this pkg list so that components can invoke createComponent with this list?

skylerjokiel · 2020-02-11T22:23:26Z

@SamBroner , the goal of this PR is simply to expose the components package path to the component itself. This will originally be used just for telemetry.

agarwal-navin · 2020-02-11T22:54:21Z

@SamBroner @skylerjokiel It could also be used to create sub components relative to the current path of a container that is created via dynamic registry (similar to how the ExternalComponentLoader is doing it now). Without the packagePath, can such a component create a sub component with a specific path?

agarwal-navin requested review from anthony-murphy and skylerjokiel February 10, 2020 21:39

agarwal-navin commented Feb 10, 2020

View reviewed changes

packages/runtime/container-runtime/src/componentContext.ts Outdated Show resolved Hide resolved

anthony-murphy reviewed Feb 10, 2020

View reviewed changes

packages/runtime/runtime-definitions/src/components.ts Outdated Show resolved Hide resolved

skylerjokiel approved these changes Feb 11, 2020

View reviewed changes

anthony-murphy reviewed Feb 11, 2020

View reviewed changes

packages/runtime/container-runtime/src/componentContext.ts Outdated Show resolved Hide resolved

anthony-murphy approved these changes Feb 11, 2020

View reviewed changes

kurtb reviewed Feb 11, 2020

View reviewed changes

agarwal-navin changed the title ~~Exposed package "type" and package "path" on IComponentContext.~~ Exposed package "path" on IComponentContext. Feb 11, 2020

agarwal-navin added 3 commits February 14, 2020 11:43

Exposed package type and package path on IComponentContext.

cb917ba

Fix for #1208.

5c17638

CR feedback.

7664a50

agarwal-navin merged commit 29d9a9f into microsoft:master Feb 14, 2020

agarwal-navin deleted the exposeTypeInComponent branch February 20, 2020 19:29

agarwal-navin mentioned this pull request May 22, 2020

Summaries: Why is a component's 'pkg' serialized as an array? #2250

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposed package "path" on IComponentContext. #1220

Exposed package "path" on IComponentContext. #1220

agarwal-navin commented Feb 10, 2020 •

edited

Loading

skylerjokiel left a comment

anthony-murphy Feb 11, 2020

anthony-murphy left a comment

kurtb Feb 11, 2020

skylerjokiel Feb 11, 2020

kurtb Feb 11, 2020

kurtb Feb 11, 2020

skylerjokiel Feb 11, 2020

agarwal-navin Feb 11, 2020

kurtb Feb 12, 2020

skylerjokiel Feb 13, 2020

kurtb Feb 11, 2020

skylerjokiel Feb 11, 2020

agarwal-navin Feb 11, 2020

skylerjokiel Feb 12, 2020

kurtb Feb 12, 2020

kurtb Feb 11, 2020

kurtb Feb 11, 2020

SamBroner commented Feb 11, 2020

skylerjokiel commented Feb 11, 2020

agarwal-navin commented Feb 11, 2020

Exposed package "path" on IComponentContext. #1220

Exposed package "path" on IComponentContext. #1220

Conversation

agarwal-navin commented Feb 10, 2020 • edited Loading

skylerjokiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anthony-murphy left a 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

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

SamBroner commented Feb 11, 2020

skylerjokiel commented Feb 11, 2020

agarwal-navin commented Feb 11, 2020

agarwal-navin commented Feb 10, 2020 •

edited

Loading