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

Add lsp-ws-connection to repo #107

Merged

Conversation

bollwyvl
Copy link
Collaborator

@bollwyvl bollwyvl commented Dec 7, 2019

References

Code changes

  • brings lsp-ws-connection into the repo
  • ts, tslint, yarn, etc.
  • also using chai instead of expect because OH MY GOD SOME DAYS I CAN'T STAND JS STUFF AT ALL
    • but seriously, it didn't seem to work properly with ts3.7
  • moves initializeParams into an overloadable protected function
    • not for any good reason, yet, but it was the first thing i wanted to do

User-facing changes

None

Backwards-incompatible changes

None yet. In a follow on, I'll probably be taking a harder look at a) whether we need it at all (MessageConnection seems just fine) and b) how we might make it more configurable by some means other than subclassing and of course c) entirely removing its knowledge of a documentUri, because we only need one websocket per language server.

Chores

  • linted
  • tested
  • documented
  • changelog entry

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

.gitignore Outdated Show resolved Hide resolved
"tslint": "tslint --fix -c tslint.json --project tsconfigbase.json 'packages/**/*{.ts,.tsx}'",
"tslint:check": "tslint -c tslint.json --project tsconfigbase.json 'packages/**/*{.ts,.tsx}'"
},
"workspaces": {
"packages": [
"packages/*"
],
"nohoist": [
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

might be able to clean these up, now that they are no longer fighting...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

still didn't look at these yet...

* Initialization parameters to be sent to the language server.
* Subclasses can overload this when adding more features.
*/
protected initializeParams(): protocol.InitializeParams {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the one thing i actually changed

* Does the server support find all references?
*/
public isReferencesSupported() {
return !!this.serverCapabilities?.referencesProvider;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

well, some of these, too...

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Dec 8, 2019

Hopefully have fixed the version detection stuff which was failing on windows. Still need to figure out why i'm having issues with menu selection, but haven't gotten my windows vm back up and running yet.

@@ -0,0 +1,29 @@
BSD 3-Clause License

Copyright (c) 2019, jupyter-lsp contributors
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should not stop at the fork mention, but also add "Copyright (c) 2018, William Conlon, ISC" in here.

Legalities aside I would like to credit @wylieconlon as it would not be possible to start this project without the leading work of @wylieconlon (and obviously lots of code in this PR is @wylieconlon code with slight modifications). @wylieconlon would you like any specific backlinks or mentions? Are you ok with the current mentions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, happy to give credit where due!

I think we may end up moving further and further from what remains of the code from upstream: for example, using tsee would let us keep the full event typing information, and remove the conflicts we're seeing between @types/node and @types/event. Or, as mentioned, just use MessageConnection more directly.

Of course, because we don't control the end-user jupyterlab webpack.config.js, we'll need to keep webpacking the part that interacts with fs, so this package (and its special needs) will continue to exist.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Dec 8, 2019

Huzzah, looks like all the CI passed, provided it can actually upload the final report bundle!

@krassowski
Copy link
Member

krassowski commented Dec 9, 2019

c) entirely removing its knowledge of a documentUri, because we only need one websocket per language server.

before that we would want to have multi-document tests; I saw a regression with inspections being displayed in a wrong file...

Otherwise, ready to merge?

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Dec 9, 2019 via email

@krassowski krassowski merged commit 5ea6977 into jupyter-lsp:master Dec 9, 2019
@krassowski
Copy link
Member

Can we synchronize the lsp-ws-connection versions between jupyterlab-lsp/package.json and metapackage/package.json? or should we have it in the former in the first place?

I stuck trying to modify lsp-ws-connection` but the changes are not reflected since it is usinga an old version.

@krassowski
Copy link
Member

I really like the idea of the monorepo (it improves workflow greatly), but I got lost with the complexity of the building/watch system and can't figure out how to get the changes in lsp-ws-connection to be reflected in mu builds... Maybe I need to rm -rf everything and install from scratch, but if there is any secret knowledge I am missing, please do help @bollwyvl.

@bollwyvl
Copy link
Collaborator Author

bollwyvl commented Dec 10, 2019 via email

@wylieconlon
Copy link

I'm glad you found the project useful! I haven't really done a full review of your usage, but the license allows forking/vendoring like you've done. Were there any bugs you encountered or other issues that could make their way back into the main repo?

@krassowski
Copy link
Member

Thank you, @wylieconlon! No, not many issues significant enough to contribute back yet, but there is a thing that I am looking into right now, related to sending messages before initialization has been completed by the server (according to the specification client cannot send any messages before that; there is a check but it is on being connected, not being initialized). Again, this is a minor thing but maybe changing:

    if (!this.isConnected) {
      return;
    }

to

    if (!this.isConnected || !this.isInitialized) {
      return;
    }

in a few places would bring the behaviour closer to the LSP specification. Also, having didClose would be nice (but I am not sure if this is relevant to the upstream project).

Also, I am experimenting on sending partial text updates and other extensions (rather than bugfixes). The ability to flexibly extend (and move away from node-based and callback-based constructs) was the primary reason to fork.

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