-
Notifications
You must be signed in to change notification settings - Fork 148
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
Use app's HTTP settings, handle feature settings race condition, CI/build updates, lazy server discovery #882
Use app's HTTP settings, handle feature settings race condition, CI/build updates, lazy server discovery #882
Conversation
👈 Launch a binder notebook on branch bollwyvl/jupyterlab-lsp/gh-880-mk2-use-server-http-settings |
…mk2-use-server-http-settings
…gh-880-mk2-use-server-http-settings
Ha, green! I can roll back the server lazy loading changes if desired. |
@@ -36,5 +36,5 @@ zip_safe = False | |||
python_requires = >=3.7 | |||
|
|||
install_requires = | |||
jupyter_lsp >=1.4.0 | |||
jupyter_lsp >=1.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is required, should we consider making a major version bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's probably worth pulling this part onto a separate PR, and doing some manual reverts.
Also, of note, we might need to be very ginger with ipykernel 6.19.*
. as i was seeing some message-related stuff that might confuse our test rig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep going an release 2.0 then. I hope to get to releasing stuff over the break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, which of the below:
- make a new PR with this HEAD (perhaps force push to [wip] defer initialization until event loop is available #852) and this roll this back to not touch server
- keep going on this, but instead of bumping to 1.6, bump to 2.0
Some background:
I guess it depends on how semver we're getting: the contract with the frontend doesn't actually change at all, but I am wary of not also testing against the oldest supported serverextension, whereas moving the pin makes it relatively clear.
The manager API does change a bit, to avoid paying for language server discovery. So if someone was overloading the manager, but using the stock routes, that could be an issue.
The listener API does not change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep going on this, but instead of bumping to 1.6, bump to 2.0
I was thinking this one. Especially if we could also get #636 in. Otherwise maybe 1.6 is fine.
So if someone was overloading the manager, but using the stock routes, that could be an issue.
Yes. I don't know of anyone who does that. I don't feel strong about this especially since we did not advertise support for overloading it like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Especially if we could also get #636 in.
That would be great.
Expounding a little further: should 2.0 be
python >=3.8
- Ipykernel, etc. have already started to move on.
jupyter_server >=2.0
- it's just going to be really messy for a while, but it will be a lot simpler to support
@@ -1,5 +1,6 @@ | |||
import * as events from 'events'; | |||
|
|||
import 'setimmediate'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed? It looks as not maintained for a while now, and I am not sure where it gets used. Is this due to a setImmediate cal in one of dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a setImmediate cal in one of dependencies?
Deep in one of the vscode-*
ones, and extremely hard to track down (hence the sourcemaps).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these instead: https://www.npmjs.com/package/timers-browserify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, uses that, too
Cool, glad to have this one in. I'll have another look at working the CI assets into the downstream on |
…p#882 Co-authored-by: Nicholas Bollweg <[email protected]>
* Use proper web socket URL, see jupyter-lsp/jupyterlab-lsp#819 Co-authored-by: Author: MikeSem <[email protected]> * Use app's HTTP settings, cherry-picked from jupyter-lsp/jupyterlab-lsp#882 Co-authored-by: Nicholas Bollweg <[email protected]> * Support optional `.virtual_documents` Cherry-picked from jupyter-lsp/jupyterlab-lsp#930 --------- Co-authored-by: Author: MikeSem <[email protected]> Co-authored-by: Nicholas Bollweg <[email protected]>
References
Code changes
/
yarn.lock
: get "one true version" oftypescript
,prettier
,webpack
.github
@jupyter-lsp/jupyterlab-lsp
LanguageServerManager
withapp.serviceManager.settings
is being usedPromiseDelegate
@jupyterlab/*
dependencies to match python-level dependency@jupyter-lsp/lsp-ws-connection
lsp-ws-connection
void
for handling of promisesjupyter_lsp
importlib(_|.)metadata
forentrypoints
User-facing changes
Backwards-incompatible changes
jupyter_lsp.LanguageServerManager
directly may need toawait
more thingsChores