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

Adding the DebuggingManager to src/platform/serviceRegistry.web.ts #10029

Closed
wants to merge 2 commits into from

Conversation

sadasant
Copy link
Contributor

Thanks to the recent PR by @rebornix #10016 and my studies on some branches (1, 2), I believe I can start working towards #9665 by ensuring the DebuggingManager runs as part of serviceRegistry.web.ts.

The migration is fairly simple, since none of the internals depend on Node.js as far as I could tell.
The changes in this PR can be tested by:

  • The web compilation (which passes).
  • Debugging via Extension (web) and confirming that anything runs on a .ipynb file (which does work).
  • The existing CI tests.

If this makes sense, this PR will be the first on a series of PRs breaking down #9665 into reasonable chunks of code.

@sadasant sadasant self-assigned this May 14, 2022
@sadasant
Copy link
Contributor Author

I'm getting:

/home/runner/work/vscode-jupyter/vscode-jupyter/src/platform/debugger/jupyter/kernelDebugAdapter.ts
39:29  error  Unexpected path "../../common/platform/types.node" imported in restricted zone. Importing node modules into non node files is not allowed  import/no-restricted-paths

However, kernelDebugAdapter.ts only depends on platform/types.node.ts for IFileSystem, which now exists on platform/types.ts, so it should be easy to fix.

I'll check eslint before pushing.

@codecov-commenter
Copy link

codecov-commenter commented May 14, 2022

Codecov Report

Merging #10029 (c603c63) into main (1a06230) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##            main   #10029   +/-   ##
======================================
  Coverage     63%      64%           
======================================
  Files        215      215           
  Lines      10122    10122           
  Branches    1612     1612           
======================================
+ Hits        6473     6479    +6     
+ Misses      3131     3123    -8     
- Partials     518      520    +2     
Impacted Files Coverage Δ
src/platform/debugger/jupyter/debugger.ts 64% <ø> (ø)
src/platform/debugger/jupyter/helper.ts 51% <ø> (ø)
src/platform/debugger/jupyter/debugControllers.ts 58% <100%> (ø)
src/platform/debugger/jupyter/debuggingManager.ts 53% <100%> (ø)
...rc/platform/debugger/jupyter/kernelDebugAdapter.ts 69% <100%> (ø)
src/platform/serviceRegistry.node.ts 97% <100%> (ø)
...rc/platform/common/dataScienceSurveyBanner.node.ts 70% <0%> (+5%) ⬆️

@sadasant sadasant changed the title Adding the DebuggingManager to the web serviceRegistry Adding the DebuggingManager to src/platform/serviceRegistry.web.ts May 14, 2022
@sadasant
Copy link
Contributor Author

I have two more commits here: https://github.com/microsoft/vscode-jupyter/compare/main...sadasant:web-DebuggingManager-extra?expand=1

There I:

  1. Add DebugService on src/platform/common/serviceRegistry.web.ts.
  2. Add DataViewerFactory and NotebookWatcher src/platform/serviceRegistry.web.ts.

Those commits seem innocuous. Should I add them to this PR?
Please let me know if I'm missing something.

@sadasant
Copy link
Contributor Author

I tried adding registerInstallerTypes(serviceManager) to src/kernels/serviceRegistry.web.ts, but this change quickly expands in scope because this service tries to install several things in the host machine, which is a problem in itself bigger than this PR.

I also tried adding PYTHON_VARIABLES_REQUESTER, MULTIPLEXING_DEBUGSERVICE, RUN_BY_LINE_DEBUGSERVICE, JupyterVariableDataProviderFactory, ALL_VARIABLES, KERNEL_VARIABLES and DEBUGGER_VARIABLES to src/kernels/serviceRegistry.web.ts but this quickly goes out of proportion. For starters, it leads to issues with EXTENSION_ROOT_DIR in src/platform/common/constants.node.ts, which should be fixed by using context.extensionUri, but context has to be routed through IExtensionContext, changing how the related services work significantly with src/platform/common/constants*.

I'd rather add these changes as part of a separate PR.

@IanMatthewHuff
Copy link
Member

@sadasant Ahh, sorry, didn't pick this up before my PR. Looks like you / me / Don were all in the same area. I believe this is now covered by my PR that went in. Do you have a PR up for the variable requestor work that I could take a peek at?

@sadasant sadasant closed this May 16, 2022
@sadasant sadasant deleted the web-DebuggingManager branch May 16, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants