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

Expose logger in web embedder API #147035

Merged
merged 18 commits into from May 11, 2022
Merged

Expose logger in web embedder API #147035

merged 18 commits into from May 11, 2022

Conversation

tanhakabir
Copy link
Contributor

@tanhakabir tanhakabir commented Apr 7, 2022

Ref: https://github.com/microsoft/vscode-dev/issues/555

@sandy081 I want to expose creating Output Channels to the embedder API but I noticed that all of Output is under vs/workbench/contrib which is not allowed to be imported in vs/workbench/browser/web.api.ts. When I exposed Progress in #147033 I didn't have any issues since progress was in vs/platform which was safe to import. Do you have any suggestions for a safe way to expose the Output API?

This exposes the OutputChannel service in the web API to be used in vscode.dev in the embedders

@sandy081
Copy link
Member

sandy081 commented Apr 8, 2022

Can I understand the use case or provide me with the context for this requirement?

@tanhakabir
Copy link
Contributor Author

@sandy081 the context in detail was explained here: https://github.com/microsoft/vscode-dev/issues/555#issuecomment-1086432256

Essentially we do work in the embedder to set up the remote connection and we'd like to be able to show progress and logs of the set up but we don't have access to the extension host in the embedder to do so today.

@sandy081
Copy link
Member

Thanks for providing the context.

It seems like workbench/contrib is not allowed in web.main.ts. Moved IOutputService to workbench/services. Please pull latest.

@tanhakabir tanhakabir marked this pull request as ready for review April 13, 2022 17:31
Copy link
Member

@connor4312 connor4312 left a comment

Choose a reason for hiding this comment

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

lgtm

@sandy081
Copy link
Member

@tanhakabir Creating an output channel is more powerful and generic API and I am not sure if the embedder wants to leverage all of it. This API also provides ability to create multiple output channels and I do not know if embedder has such a need.

If this is embedder log, then how about exposing following logging api to the embedder

log(level: LogLevel, message: string)

Workbench will implement this API and logs messages as per log level. If wanted, workbench can create a separate logger for embedder.

@bpasero
Copy link
Member

bpasero commented Apr 15, 2022

I think we need to start thinking about a better place where the embedder API is implemented, I do not think that workbench core is good, esp. since we have a contrib model typically. I am open for suggestions.

@tanhakabir
Copy link
Contributor Author

@bpasero I haven't worked within the vscode repo much yet so I don't have the best context but I agree that the contrib model would make more sense here with the WebEmbedderLog. To clarify though did you mean just the logger should follow the contrib model or some larger refractoring to web.main.ts and web.factory.ts to follow other models in vscode?

@bpasero
Copy link
Member

bpasero commented Apr 16, 2022

I think we cannot avoid having the API all in one place, but maybe the implementation could be spread to respective components. E.g. the output component can apply its properties to the embedder API object.

@tanhakabir
Copy link
Contributor Author

Closing in favor of #147728

@tanhakabir tanhakabir closed this Apr 19, 2022
@tanhakabir
Copy link
Contributor Author

As discussed in #147728 (comment) reopening this PR for the April iteration to get access to a logger in the embedder. cc @bpasero

@tanhakabir tanhakabir reopened this Apr 25, 2022
@tanhakabir tanhakabir requested a review from bpasero April 25, 2022 17:15
@tanhakabir tanhakabir changed the title Expose output channel in web embedder API Expose logger in web embedder API Apr 25, 2022
src/vs/workbench/browser/parts/log/webEmbedderLog.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/parts/log/webEmbedderLog.ts Outdated Show resolved Hide resolved
src/vs/workbench/browser/web.main.ts Show resolved Hide resolved
@sandy081
Copy link
Member

@tanhakabir I pushed following changes

  • Introduced embedder logger
  • Exposed log api for embedder to log to embedder logger
  • Registered embedder logger into output channel only when logging is attempted - this way this log channel is not shown until logging is done

I agree with @bpasero's comment on namespace for log API. How about moving this API into log or embedder namespace?

@sandy081
Copy link
Member

Since we are in endgame week already, shall we push this to next milestone or is there an urgency to push it for this milestone?

@bpasero
Copy link
Member

bpasero commented Apr 26, 2022

Looks much cleaner how, thanks for chiming in ❤️

@tanhakabir
Copy link
Contributor Author

We can push this into the next iteration but early in the iteration is preferable.

@tanhakabir tanhakabir added this to the May 2022 milestone Apr 26, 2022
@sandy081
Copy link
Member

Great. We can push it as soon as the main is open for May milestone. Meanwhile can you please test these changes?

@tanhakabir
Copy link
Contributor Author

Pushed a small addition but otherwise looks great! Thanks so much for your help @sandy081! ❤️

src/vs/workbench/browser/web.api.ts Outdated Show resolved Hide resolved
@tanhakabir tanhakabir requested a review from bpasero May 10, 2022 04:09
@tanhakabir tanhakabir requested a review from sandy081 May 10, 2022 15:45
@tanhakabir tanhakabir dismissed bpasero’s stale review May 11, 2022 16:30

Made the change you requested but you're on paternity leave and I was hoping to get this PR in soon so I want to dismiss this review

@tanhakabir tanhakabir merged commit 2872af5 into main May 11, 2022
@tanhakabir tanhakabir deleted the tanha/embedder-output branch May 11, 2022 16:31
@github-actions github-actions bot locked and limited conversation to collaborators Jun 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants