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

feat(lsp): jupyter notebook analysis #20719

Merged
merged 8 commits into from
Sep 29, 2023
Merged

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Sep 28, 2023

All of our analysis and editor features will work in vscode notebook cells with this.

About 5/6 of this PR is making sure we re-normalize module specifiers that come out of tsc, since notebook cell URIs have to be transformed on the way in.

Coupled with denoland/vscode_deno#949.
Fixes #20657.
Fixes .. #20662.

Todo (not in this PR):

@rgbkrk
Copy link
Contributor

rgbkrk commented Sep 28, 2023

This doesn't close #20662, as that's for supporting the extended Jupyter protocol completions (separate from LSP) when attached to a Jupyter Frontend (Jupyterlab, jupyter console).

Awesome to see for us VS Code users though! Thank you.

@nayeemrmn
Copy link
Collaborator Author

  • Local variables aren't remembered between cells. They're essentially different modules.

Interesting update! It seems the regular node/ts server does the same thing, for each cell that contains import or export. It just tries to keep the rest in script mode. But even then it doesn't validate the ordering at all.

With that in mind I think this is ready to land as an MVP.

@nayeemrmn nayeemrmn marked this pull request as ready for review September 28, 2023 04:34
@nayeemrmn nayeemrmn requested a review from dsherret September 28, 2023 04:36
@nayeemrmn
Copy link
Collaborator Author

  • Sometimes you get a silent tsc error because the cell URLs don't end with a .ts extension. We have a specifier denormalization layer for that but it's tricky because we currently don't normalize everywhere we should, and it needs to take into account the out-of-band media type provided with textDocument/didOpen.

Done now: 3d5e434. This normalizes all module specifiers that come out of tsc, and denormalizes notebook cell specifiers that go into tsc (by converting them to file URLs and forcing a file extension).

This basically has parity with modules now. All quick-fixes, local and remote import completion, auto-imports etc. work as far as I can see. It's in a pretty complete state apart from:

  • Local variables aren't remembered between cells. They're essentially different modules.

which is the same case with the node/tsc server.

@nayeemrmn nayeemrmn changed the title feat(lsp): jupyter notebook analysis MVP feat(lsp): jupyter notebook analysis Sep 29, 2023
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

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

LGTM!

cli/lsp/documents.rs Show resolved Hide resolved
cli/lsp/tsc.rs Outdated Show resolved Hide resolved
@nayeemrmn nayeemrmn merged commit 2d1af0c into denoland:main Sep 29, 2023
12 checks passed
@nayeemrmn nayeemrmn deleted the lsp-jupyter branch September 29, 2023 19:45
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.

Autocompletion doesn't work in VSCode jupyter notebooks + deno kernel
3 participants