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

8965-hosted-plugin-outfiles #9176

Merged
merged 1 commit into from
May 26, 2021
Merged

8965-hosted-plugin-outfiles #9176

merged 1 commit into from
May 26, 2021

Conversation

danarad05
Copy link
Contributor

Hosted Plugin debug session contained entire directory of plugin instead of only the out directory, which resulted in performance warning because it would load all files in that directory.

Remarks:

  1. vscode-js-debug is the library that throws the warning.
  2. I tried to increase the timeout till warning but it didn't solve issue when plugin contains a substantial amount of files.

Signed-off-by: Dan Arad [email protected]

What it does

Fixes: #8965

How to test

  1. Run Theia, open an extension directory - i.e. vscode-extension-samples\custom-editor-sample
  2. Run Hosted Plugin: Start Instance and try to debug extension

Review checklist

Reminder for reviewers

@amiramw
Copy link
Member

amiramw commented Mar 11, 2021

@danarad05 are you still able to debug ts files without the sources?

@danarad05
Copy link
Contributor Author

danarad05 commented Mar 11, 2021 •
edited
Loading

@danarad05 are you still able to debug ts files without the sources?

Yes, debugged extension successfully - an extension that prior to this change wasn't debugeable because vscode-js-debug didn't complete loading all extension js files - event after increasing timeout to 100s.

And the sources exist in out folder...

@danarad05
Copy link
Contributor Author

@vince-fugnitto who could review this? Thanks

@vince-fugnitto vince-fugnitto added debug issues that related to debug functionality plug-in system issues related to the plug-in system labels Mar 31, 2021
@paul-marechal
Copy link
Member

I think it might be ok temporarily. IIUC this hosted plugin debug feature is to debug extensions developed within a Theia application? We need your change because users can't configure outFiles themselves, since this is hardcoded here when starting the debug session...

Looking at how VS Code does things they have a specific extensionHost debug type for this: https://github.com/microsoft/vscode-generator-code/blob/main/generators/app/templates/ext-command-ts/vscode/launch.json

I wasn't able to find where this debug configuration is contributed in the VS Code repo, but I wouldn't be surprised if it was one of the proprietary bits from VS Code.

In the long run I think it would be best to have the same type of debug configuration, this way developers can configure everything more specifically rather than forcing everyone to use the out folder pattern.

@danarad05 danarad05 force-pushed the 8965-hosted-plugin-outfiles branch 3 times, most recently from 9c41350 to 1dd3879 Compare April 4, 2021 10:55
paul-marechal
paul-marechal approved these changes May 5, 2021
View reviewed changes
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@paul-marechal
Copy link
Member

Can you restart CI to see if it turns green?

@danarad05
Copy link
Contributor Author

Can you restart CI to see if it turns green?

Sorry, I don't have required privileges.

@vince-fugnitto
Copy link
Member

Sorry, I don't have required privileges.

@danarad05 please rebase and amend the pull-request it should re-trigger CI. For some reason the 're-run all jobs' option is not present on the github actions page for this pull-request.

@danarad05 danarad05 force-pushed the 8965-hosted-plugin-outfiles branch from 1dd3879 to 93c4f39 Compare May 20, 2021 09:59
@danarad05
Copy link
Contributor Author

danarad05 commented May 20, 2021 •
edited
Loading

@vince-fugnitto

  1. Rebased - builds are successful.
  2. In continuation of my remark here - I would like to complete this PR review ASAP.

Thanks

@danarad05
8965-hosted-plugin-outfiles
6262d8c 
Hosted Plugin debug session contained entire directory of plugin instead of only the out directory containing it - which resulted in performance warning because it would load all files in that directory.
Also, added to preferences ability to configure launch `outFiles`.

Signed-off-by: Dan Arad <[email protected]>
@danarad05 danarad05 force-pushed the 8965-hosted-plugin-outfiles branch from 26b397c to 6262d8c Compare May 25, 2021 13:35
@danarad05 danarad05 requested a review from paul-marechal May 25, 2021 13:38
vince-fugnitto
vince-fugnitto approved these changes May 26, 2021
View reviewed changes
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look fine to me 👍

@vince-fugnitto
Copy link
Member

@danarad05 we merge for the release? :)

@danarad05
Copy link
Contributor Author

@danarad05 we merge for the release? :)

Yes please

@vince-fugnitto vince-fugnitto merged commit 6e15b35 into eclipse-theia:master May 26, 2021
@vince-fugnitto vince-fugnitto added this to the 1.14.0 milestone May 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vince-fugnitto vince-fugnitto vince-fugnitto approved these changes

@paul-marechal paul-marechal Awaiting requested review from paul-marechal

Assignees
No one assigned
Labels
debug issues that related to debug functionality plug-in system issues related to the plug-in system
Projects
None yet
Milestone
1.14.0
Development

Successfully merging this pull request may close these issues.

Debug hosted plugin doesn't stop at breakpoints on Gitpod
4 participants
@danarad05 @amiramw @paul-marechal @vince-fugnitto