Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Expose vue app instance for extensions #12629

Closed
wants to merge 1 commit into from
Closed

Expose vue app instance for extensions #12629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion shell/core/plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ export default function(context, inject, vueApp) {
store,
$axios,
redirect,
plugins: this
plugins: this,
vueApp, // Expose app instance to plugins to allow adding components and such
Copy link
Member

@richard-cox richard-cox Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you expand on the use cases this solves?

There's a lot of power from extensions having access to this which is i think why it was omitted. If that was the case could the same use cases be covered by beefing up the plugin api?

Copy link
Contributor Author

@cnotv cnotv Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in the original configuration, it was specifically passed the attribute and we have cut it out inadvertently. It was not used anywhere and that was the reason we did not notice it.

As I have described in the issue, it would allow to use the Vue App API:

export declare interface App<HostElement = any> {
    version: string;
    config: AppConfig;
    use<Options extends unknown[]>(plugin: Plugin_2<Options>, ...options: Options): this;
    use<Options>(plugin: Plugin_2<Options>, options: Options): this;
    mixin(mixin: ComponentOptions): this;
    component(name: string): Component | undefined;
    component(name: string, component: Component): this;
    directive(name: string): Directive | undefined;
    directive(name: string, directive: Directive): this;
    mount(rootContainer: HostElement | string, isHydrate?: boolean, isSVG?: boolean): ComponentPublicInstance;
    unmount(): void;
    provide<T>(key: InjectionKey<T> | string, value: T): this;
}

I had a case where I needed to extend the app. In the specific case, I needed config.errorHandler to be mapped because this is how it's handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are interested in a case where third parties may want to use the extensions, this is one: https://docs.sentry.io/platforms/javascript/guides/vue/#vue-3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, could you describe a case where you would not allow this? I could not think of anything specific.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, in the original configuration, it was specifically passed the attribute and we have cut it out inadvertently.

I couldn't find where this was removed, would you be able to provide the commit?

I had a case where I needed to extend the app. In the specific case, I needed config.errorHandler to be mapped because this is how it's handled.
Out of curiosity, could you describe a case where you would not allow this?

I think the plan is to keep a very clean contract between the dashboard via the extension by a well defined API. Anything that the extension needs to change, supplement, override, etc should ideally be via that API. By providing them access to the vueApp (and others objects in internal) we're exposing the dashboard to extensions doing weird and wonderful things. So lots of utility, but also lot of problem. The comment above this block of code roughly covers this concept Plugins should not use these - but we will pass them in for now as a 2nd argument in case there are use cases not covered that require direct access - we may remove access later

Copy link
Contributor Author

@cnotv cnotv Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find where this was removed, would you be able to provide the commit?

The code was already missing the implementation related to it, so we may talk about 4 years ago?

we're exposing the dashboard to extensions doing weird and wonderful things. So lots of utility, but also lot of problem

For instance?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have cut it out inadvertently

The code was already missing the implementation related to it

I'm not following. It was never wired in, or it was and then removed? Ultimately was looking for breadcrumbs on design decisions.

we're exposing the dashboard to extensions doing weird and wonderful things. So lots of utility, but also lot of problem

For instance

Extensions that depend on root objects could lead to ...

  • inadvertently or intentionally breaking the UI in a serious way, error handling primarily but anything else that the dashboard depends or via those objects
  • extensions suffer breaking changes given intentional dashboard changes. for instance
    • we update vue and vueApp changes....
    • we replace the vuex store / upgrade it in a breaking way
  • extensions suffer breaking changes given unintentional dashboard changes given updates to those objects.
    • extension developers would need to stay up to date with all breaking changes in whatever we pass in, rather than the UI handles those our side of the api unbeknown to extension developer

Restating that architectural point, extensions should interact with the dashboard via well defined api rather than root environment objects, and that the internal object shouldn't be used by extensions and will eventually be removed.

For the record i'm not saying there's no way to integrate sentry via extension, just that it could be done via dedicated api method (maybe a way to inject something into the pipeline in shell/initialize/entry-helpers.js)

If you wish to discuss this further lets have a call with myself, Neil, Alex and Jordan when I'm back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All your points are correct, but breaking the application without exposing the instance is still possible.
I already thought about passing around the initialization, but it must still map the error handler, which needs the instance.

This was needed for the hack week, but given the circumstances, I leave it to you guys to plan and find a way that better fits your concept. At least I've raised awareness about it.

Just for the record, the implementation works:

Screenshot 2024-11-21 at 14 25 52

};

return internal;
Expand Down
3 changes: 2 additions & 1 deletion shell/initialize/install-plugins.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ export async function installInjectedPlugins(app, vueApp) {
if (typeof pluginDefinition === 'function') {
await pluginDefinition(
app.context,
(key, value) => inject(key, value, app.context, vueApp)
(key, value) => inject(key, value, app.context, vueApp),
vueApp, // Expose app instance to plugins to allow adding components and such
);
}
});
Expand Down
Loading