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

Rename HostedPluginServer, etc. #10352

Merged

Conversation

tsmaeder
Copy link
Contributor

@tsmaeder tsmaeder commented Oct 29, 2021

Signed-off-by: Thomas Mäder [email protected]

What it does

Fixes: #10351

Renames HostedPluginServer and related symbols.

How to test

Make sure running plugins from source in a second instance of Theia still works ("Hosted Plugin: Debug Instance", etc.)

Review checklist

Reminder for reviewers

Signed-off-by: Thomas Mäder <[email protected]>
@vince-fugnitto vince-fugnitto added plug-in system issues related to the plug-in system quality issues related to code and application quality labels Oct 29, 2021
@colin-grant-work colin-grant-work added the potentially-breaking A change that might break some dependents conditionally label Oct 29, 2021
@colin-grant-work
Copy link
Contributor

Could use a changelog entry, since anyone relying on the current name will see breakage.

@@ -240,7 +240,7 @@ export class JsonRpcProxyFactory<T extends object> implements ProxyHandler<T> {
try {
if (isNotify) {
connection.sendNotification(method, ...args);
resolve();
resolve(undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Rather change line 239 to be new Promise<void>(...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the promise can yield a result if it's not a notification.

Copy link
Member

Choose a reason for hiding this comment

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

OK I had not seen the code around it! Though is this required by TypeScript? It's often fine to just write resolve() when nothing is returned, and resolve(undefined) is usually a smell that the promise constructor is not aptly typed.

@tsmaeder
Copy link
Contributor Author

tsmaeder commented Nov 8, 2021

I'll get back to this one once I have time....

Signed-off-by: Thomas Mäder <[email protected]>
@tsmaeder
Copy link
Contributor Author

Ping @paul-marechal

@@ -1,5 +1,5 @@
# Change Log

- [plugindev] Renamed HostedPlugin(Server|Client) to PluginDev(Server|Client)
Copy link
Member

Choose a reason for hiding this comment

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

I can fix it when I do the release changelog, but it should be in the form:

## v1.20.0 - 11/25/2021

[1.20.0 Milestone](https://github.com/eclipse-theia/theia/milestone/28)

<a name="breaking_changes_1.20.0">[Breaking Changes:](#breaking_changes_1.20.0)</a>

-  [plugin] renamed `HostedPluginServer` and `HostedPluginServer` to `PluginDevServer` and `PluginDevClient` respectfully [#10352](https://github.com/eclipse-theia/theia/pull/10352)

@paul-marechal paul-marechal merged commit 6b9e099 into eclipse-theia:master Nov 23, 2021
@vince-fugnitto vince-fugnitto added this to the 1.20.0 milestone Nov 25, 2021
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 potentially-breaking A change that might break some dependents conditionally quality issues related to code and application quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename HostedPluginServer
4 participants