-
Notifications
You must be signed in to change notification settings - Fork 262
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.
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
Expose vue app instance for extensions #12629
Conversation
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
Closing due to blocking issues. |
Summary
Fixes #12628
Occurred changes and/or fixed issues
Logic already exists and exposing the Vue instance should not cause any harm.
Technical notes summary
Areas or cases that should be tested
Areas which could experience regressions
Screenshot/Video
Checklist