plugin-dev - fix run/debug flow to work on windows + code clean #5608
Conversation
amiramw
commented
Jun 28, 2019
•
amiramw
commented
- use fileSystem.getFsPath utility for HOSTED_PLUGIN as it also supports windows
- fixed typos (one in HostedInstanceState enum is added to changelog)
- remove unused code
- add missing dependencies in package.json
packages/plugin-dev/src/browser/hosted-plugin-manager-client.ts
Outdated
Show resolved
Hide resolved
| @@ -97,8 +99,7 @@ delete PROCESS_OPTIONS.env.ELECTRON_RUN_AS_NODE; | |||
| @injectable() | |||
| export abstract class AbstractHostedInstanceManager implements HostedInstanceManager { | |||
| protected hostedInstanceProcess: cp.ChildProcess; | |||
| protected processOptions: cp.SpawnOptions; | |||
There was a problem hiding this comment.
Why is processOptions removed?
There was a problem hiding this comment.
it is not used. should i keep it anyway?
There was a problem hiding this comment.
Same reason as #5608 (comment)
There was a problem hiding this comment.
as this member is never initialized or used I guess no one should use it. Updated the changelog.
There was a problem hiding this comment.
You cannot know it. Theia is a framework, there are plenty clients ouside of this repo. Please keep changes related to an issue. It seems to be one-liner to use FileUri.fsPath instead of toString.
|
@amiramw can you provide some description to the PR and also the commit message? |
|
I now added more informative description to commit and PR. |
|
@evidolob please review breaking changes, are you fine with them or removed members are used by Che? |
- use FileUri.fsPath utility for HOSTED_PLUGIN as it also supports windows - fixed typos (one in HostedInstanceState enum is added to changelog) - removed member `processOptions` from `AbstractHostedInstanceManager` as it is not initialized or used (changelog updated) - add missing dependencies in package.json Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
