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

Remove @optional usage in terminal #127878

Closed
4 of 8 tasks
Tyriar opened this issue Jul 2, 2021 · 1 comment · Fixed by #136368
Closed
4 of 8 tasks

Remove @optional usage in terminal #127878

Tyriar opened this issue Jul 2, 2021 · 1 comment · Fixed by #136368
Assignees
@Tyriar
Labels
debt Code quality issues terminal Integrated terminal issues
Milestone
October 2021

Comments

@Tyriar
Copy link
Member

Tyriar commented Jul 2, 2021 •
edited

We currently use @optional for ILocalTerminalService which is an electron-only service which obviously may not exist. Things to look at:

  • Consider merging local and remote services
    • Allow optionally passing in remoteAuthority to use the non-default connection
    • Use overloads for createProcess
    • Throw/error out when using local but it's not registered
  • Work around electron-sandbox problem with a contribution that registers itself to the new service?
  • Remove ILocalTerminalService from platform/terminal
  • Consider removing the service all together in favor of passing it directly such that no other component can rely on this internal to the terminal service
  • Look at ILocalPtyService

Part of #119440
@Tyriar Tyriar added debt Code quality issues terminal Integrated terminal issues labels Jul 2, 2021
@Tyriar Tyriar added this to the July 2021 milestone Jul 2, 2021
Tyriar added a commit that referenced this issue Jul 8, 2021
@Tyriar
Move local/offproc service interfaces out of platform
db1bbe7 
Part of #127878
Tyriar added a commit that referenced this issue Jul 8, 2021
@Tyriar
Add warning about ILocalPtyService
3ea4f3a 
Since it's restricted to the electron-sandbox layer it can only be used when valid so
there's no concern about needing @optional here. The warning is to discourage any
usage outside of the terminal as this service deals with raw events to manage/
manipulate ptys. Without going through the TerminalService this could create processes
but they wouldn't get managed or displayed.

Part of #127878
@Tyriar Tyriar modified the milestones: July 2021, August 2021 Jul 20, 2021
@Tyriar Tyriar modified the milestones: August 2021, September 2021 Aug 19, 2021
@Tyriar
Copy link
Member Author

Tyriar commented Oct 25, 2021

Tracking remaining bits in #119440

@Tyriar Tyriar closed this as completed Oct 25, 2021
Tyriar added a commit that referenced this issue Nov 3, 2021
@Tyriar
Start replacing ILocalTerminalService with a registered backend
b52be01 
Part of #127878
@Tyriar Tyriar mentioned this issue Nov 3, 2021
Merged
@github-actions github-actions bot locked and limited conversation to collaborators Dec 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@Tyriar Tyriar

Labels
debt Code quality issues terminal Integrated terminal issues
Projects
None yet
Milestone
October 2021
2 participants
@Tyriar @meganrogge