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.

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

[plugin] handle plugin host activation exceptions #8103

Merged
merged 1 commit into from
Jun 29, 2020
Merged

Conversation

thegecko
Copy link
Member

Signed-off-by: Rob Moran rob.moran@arm.com

What it does

Resolves #8018

Added catch to plugin host to handle exceptions thrown while resolving dependencies in plugins.

  • Activation promise rejected if it exists
  • Exception logged and shown to user in notification.

How to test

Follow steps in #8018

Review checklist

Reminder for reviewers

@thegecko thegecko requested a review from akosyakov June 28, 2020 20:38
@akosyakov akosyakov added the plug-in system issues related to the plug-in system label Jun 29, 2020
@akosyakov akosyakov requested a review from benoitf June 29, 2020 07:01
this.pluginActivationPromises.get(plugin.model.id)!.reject(err);
}
const message = `Activating extension '${plugin.model.displayName || plugin.model.name}' failed: ${err.message}`;
this.messageRegistryProxy.$showMessage(MainMessageType.Error, message, {}, []);
Copy link
Member

@akosyakov akosyakov Jun 29, 2020

Choose a reason for hiding this comment

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

I was wondering maybe instead of using message service, we should have a special service then you can custoimize it in your product to completely stop the execution if some core extensions are failed, by default it will delegate to the notification center.

Copy link
Member Author

@thegecko thegecko Jun 29, 2020

Choose a reason for hiding this comment

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

I like this idea, but it feels like a separate PR to change architecture rather than this bug fix.
I'll consider as the approach for our situation if that's OK?

Copy link
Member

Choose a reason for hiding this comment

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

ok

Signed-off-by: Rob Moran <rob.moran@arm.com>
@akosyakov
Copy link
Member

code looks good, but I have to check 2 cases:

  • some vs code extensions stub main field in package.json if they don't have any code, we should not notify user about such
  • some vs code extensions don't have main field in package.json if they don't have any code, we should not notify user about such as well

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I've tested with How to test from #7852 and it seems to work

@thegecko thegecko merged commit a8bd163 into master Jun 29, 2020
@thegecko thegecko deleted the plugin-warning branch June 29, 2020 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[plugin] module resolution needs to be fully qualified
2 participants