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

Support executing debug in the browser #10748

Merged
merged 1 commit into from
Jun 21, 2022

Conversation

thegecko
Copy link
Member

@thegecko thegecko commented Feb 14, 2022

Signed-off-by: robmor01 [email protected]

What it does

  • Moved a lot of the debug files into common areas as they aren't node-specific
  • Support execution of debug in the frontend by splitting the executable resolver/starter into frontend and backend
  • The frontend version only supports creating an inline debug adapter

How to test

I've created a modified vscode.mock-debug which only runs as a web extension. Download here:

https://github.com/thegecko/vscode-mock-debug/releases/tag/web-1.0.0

Drop it into the plugins folder and run Theia...

Screenshot 2022-02-14 at 10 25 29

Review checklist

Reminder for reviewers

@vince-fugnitto vince-fugnitto added debug issues that related to debug functionality vscode issues related to VSCode compatibility labels Feb 14, 2022
@thegecko thegecko force-pushed the support-browser-debug branch 4 times, most recently from 6e92ea1 to 7ef5794 Compare February 18, 2022 11:04
Copy link
Member

@msujew msujew 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 are looking quite good to me. Please document the moved files and everything else in the breaking changes section for this release.

  • Debugging in the browser using the provided extension works correctly
  • Normal debug providers continue to work as expected (Tested with JS debugger for Node.js)

* @param contributions available debuggers contributions
*/
registerDebuggersContributions(pluginFolder: string, contributions: PluginPackageDebuggersContribution[]): void {
registerDebuggersContributions(pluginFolder: string, pluginType: theia.PluginType, contributions: PluginPackageDebuggersContribution[]): void {
Copy link
Member

Choose a reason for hiding this comment

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

Needs a breaking change in the CHANGELOG.md

@tsmaeder
Copy link
Contributor

I could use a bit more description what the idea of this change is. Are you enabling front end plugins to contribute debug adapters?

@thegecko
Copy link
Member Author

thegecko commented Feb 22, 2022

I could use a bit more description what the idea of this change is. Are you enabling front end plugins to contribute debug adapters?

Yes, see the example debug extension linked above

@thegecko thegecko force-pushed the support-browser-debug branch 2 times, most recently from 1e3a0e9 to e58a2a7 Compare February 28, 2022 16:06
@thegecko
Copy link
Member Author

@msujew Added changes to the CHANGELOG.md as requested

@thegecko
Copy link
Member Author

@msujew @tsmaeder Is there anything more required to help you test this PR or satisfy your comments?

@tsmaeder
Copy link
Contributor

Never mind me, won't have time to do a real review on this one.

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

I'm fine with the changes performed here.

Just one more minor comment: Do you mind reinstating the moved files and simply let them re-export the exports from the common folder. That way, it doesn't have to be a breaking change. Additionally, you can add a @deprecated comment so that users switch to the implementation exported from common.

@thegecko
Copy link
Member Author

From our side, the only thing I'm keen to add is to bring a lot of the debug management into the frontend so other extensions play nicely (e.g. viewers which tie into readMemory/writeMemory requests). As it currently stands, implementing #10841 wouldn't work for frontend debuggers.

However, I'd like to make this change in a separate PR

@thegecko
Copy link
Member Author

Do you mind reinstating the moved files and simply let them re-export the exports

Hmm, happy to, but:

  • Does every class and public property need this indirection? Which parts of the debug stack are the API?
  • We would need to remember to remove this later, does it make the code messy in the interim and require a further ticket?
  • What happens if/when this is refactored to bring the main functionality into the browser (as mentioned in my last comment), double-hop exports?
  • Is a deprecated warning enough? With these kept in place, developers could still get into a pickle adding node dependencies in the browser, perhaps?

Not against this change, just interested in your thoughts on the above @msujew

@msujew
Copy link
Member

msujew commented Mar 17, 2022

Does every class and public property need this indirection? Which parts of the debug stack are the API?

I believe so, since someone might depend on anything that's somehow exported.

We would need to remember to remove this later, does it make the code messy in the interim and require a further ticket?

Ideally, the the export code should just look like this:

/**
 * @deprecated since 1.24.0. - Moved to `common`. Import from there, instead.
 */
export * from '../common/...';

Not really messy, and can easily be removed a few versions down the line.

What happens if/when this is refactored to bring the main functionality into the browser (as mentioned in my last comment), double-hop exports?

Well, that won't work. But that wouldn't require moving existing parts from common to browser anyway, would it?

Is a deprecated warning enough? With these kept in place, developers could still get into a pickle adding node dependencies in the browser, perhaps?

Well these exports are just there to prevent us from instantly breaking existing backend debuggers, so that devs have a larger buffer to update their code base. They are completely justified in using node dependencies in that case.

Perhaps I don't understand you correctly. Do you plan to lift the whole debugging lifecycle into the browser frontend in the future?

@thegecko
Copy link
Member Author

This PR is ready for re-review.

It has been rebased on the good work from @colin-grant-work and all conflicts resolved. Functionality re-tested.

@msujew , apologies it has taken so long to address your comments. All moved functions/exports/classes now have export sin their original locations with deprecation notices to maintain the API

Keen to see this merged!

@colin-grant-work
Copy link
Contributor

I'll take a look later today or Monday.

/**
* It is supposed to work at node only.
* @deprecated since 1.27.0. - Moved to `debug`. Import from there, instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should note the change of filename to debug-ext.

@colin-grant-work
Copy link
Contributor

colin-grant-work commented Jun 20, 2022

Just one more minor comment: Do you mind reinstating the moved files and simply let them re-export the exports from the common folder. That way, it doesn't have to be a breaking change. Additionally, you can add a @deprecated comment so that users switch to the implementation exported from common.

@msujew, I'd be inclined to remove the reexports for several reasons. (1) Since we're going to start doing stable releases later this year, any deprecations now may be longer-lived than they might otherwise be, so it may be better to break now before we commit to some kind of stability. (2) The architecture of the plugin system isn't very conducive to overrides, so it's relatively less likely that downstream apps interact with these classes very much. (3) Deprecations of this kind don't show up very well in IDE's (/** @deprecated */ export * from '...' doesn't show a deprecation decoration in VSCode if you import from the old location, e.g.), so unless downstream apps are conscientious about scanning for new @deprecation markers or we mention the deprecation in the changelog, downstream apps are unlikely to notice until we break and delete these, anyway. (4) Breaking and renaming rather than renaming and reexporting is easier to see in git history, since then git can identify the renaming as such (coincidentally, that would also be easier to review ;-)), so anyone looking through the history that way would have a better chance of identifying what happened and why.

On the other hand, it does produce a fair bit of breakage, so there's certainly still some argument for the reexports. What do you think? One alternative would be no reexports in the plugin system, but reexports in debug, since debug follows our normal DI patterns more closely and is more likely to be extended / modified downstream?

@msujew
Copy link
Member

msujew commented Jun 20, 2022

These are some good arguments @colin-grant-work! I wouldn't insist on the reexports, so I'd be fine with simply removing them 👍

@thegecko
Copy link
Member Author

thegecko commented Jun 20, 2022

Shall I remove the re-exports, then? @colin-grant-work ?

@colin-grant-work
Copy link
Contributor

@thegecko, yes please. Sorry for the back and forth on the question! Ping me when you're done, and I'll run through one last time and approve.

Signed-off-by: thegecko <[email protected]>
@thegecko
Copy link
Member Author

@thegecko, yes please. Sorry for the back and forth on the question! Ping me when you're done, and I'll run through one last time and approve.

No probs, latest push has removed the original files

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

The new browser adapters are working well, and the old functionality is intact. 👍

@thegecko thegecko merged commit b337a32 into eclipse-theia:master Jun 21, 2022
@thegecko thegecko deleted the support-browser-debug branch June 21, 2022 06:25
@vince-fugnitto vince-fugnitto added this to the 1.27.0 milestone Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
debug issues that related to debug functionality vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants