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

Update to the latest vscode-css-languageserver #13

Merged
merged 11 commits into from
Jun 14, 2020
Merged

Update to the latest vscode-css-languageserver #13

merged 11 commits into from
Jun 14, 2020

Conversation

jfcherng
Copy link
Collaborator

@jfcherng jfcherng commented Jun 7, 2020

The server executable has been changed from out/cssServerMain.ts to out/node/cssServerMain.ts in the upstream about 16 days ago.


Other than that, I would also like to introduce another change to this (and other NPM-based LSP-* packages). That is, making this plugin less dependent on the server name (i.e., vscode-css, vscode-html etc...).

  • Change vscode-css/, vscode-html/ etc... into just language-server/
  • Change compile-vscode-css-languageserver.sh into just compile-language-server.sh
  • Mention which language server is used in the README

I think these changes should make it less work to create yet another NPM-based plugin by copy-paste. But I am free to revert this change, the major goal of this PR is to fix the changed upstream server path.

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
Signed-off-by: Jack Cherng <jfcherng@gmail.com>
- Change vscode-css/ into just language-server/
- Change compile-vscode-css-languageserver.sh into just compile-language-server.sh
- Mention which language server is used in the README

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
Copy link
Member

@rchl rchl left a comment

Choose a reason for hiding this comment

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

I think the build script should also clean up after itself - leave only the compiled version.

Something like I did in https://github.com/sublimelsp/LSP-eslint/blob/5ec75d65d1b235071a695d1b69829b883b07dcee/compile-server.sh

README.md Outdated Show resolved Hide resolved
language-server/README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
language-server/README.md Outdated Show resolved Hide resolved
@rchl
Copy link
Member

rchl commented Jun 7, 2020

Other than that, I would also like to introduce another change to this (and other NPM-based LSP-* packages). That is, making this plugin less dependent on the server name (i.e., vscode-css, vscode-html etc...).

That sounds good to me.

@rchl
Copy link
Member

rchl commented Jun 7, 2020

I kinda would wish to know from which to which commit we are updating now so that I could look through all the commits and check for any changes / potential new settings....

And I'm not saying you should tell me but I'm thinking about a potential solution for that like automatically storing commit hash on updating. Put I think that currently, we don't have that info (when pulling master).

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
@jfcherng
Copy link
Collaborator Author

jfcherng commented Jun 7, 2020

I think the build script should also clean up after itself - leave only the compiled version.

Not sure what this means. Delete the downloaded file/dir?

@rchl
Copy link
Member

rchl commented Jun 7, 2020

I think the build script should also clean up after itself - leave only the compiled version.

Not sure what this means. Delete the downloaded file/dir?

Yes, that's what I'm thinking.

README.md Outdated Show resolved Hide resolved
@jfcherng
Copy link
Collaborator Author

jfcherng commented Jun 7, 2020

I kinda would wish to know from which to which commit we are updating now so that I could look through all the commits and check for any changes / potential new settings....

And I'm not saying you should tell me but I'm thinking about a potential solution for that like automatically storing commit hash on updating. Put I think that currently, we don't have that info (when pulling master).

The simplest solution maybe move from "download the zipped tarball" to "clone the whole repo and then checkout" (and we don't have to clean up the downloaded repo, which can be reused next time). What do you think?

@rchl
Copy link
Member

rchl commented Jun 7, 2020

The simplest solution maybe move from "download the zipped tarball" to "clone the whole repo and then checkout" (and we don't have to clean up the downloaded repo, which can be reused next time). What do you think?

I'm fine with using git but I'd still want to clean up I think. Otherwise, all those files are copied to the ST cache (when developing) and I'd like to avoid that.

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
@jfcherng
Copy link
Collaborator Author

jfcherng commented Jun 7, 2020

@rchl Luckily, the commit hash is in the comment of the downloaded .zip file (but not in .tar.gz) and unzip -z file.zip outputs it. See e71c3ff#diff-f12eb39acb360e835c9399c7b0fbda8aR26-R39

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
Signed-off-by: Jack Cherng <jfcherng@gmail.com>
@rchl
Copy link
Member

rchl commented Jun 7, 2020

If you are gonna be merging this, please change the summary to say that it's about updating the server to the latest version as that is really the main reason for path "fix".

@jfcherng jfcherng changed the title Fix vscode-css language server path Update to the latest vscode-css-languageserver Jun 7, 2020
Signed-off-by: Jack Cherng <jfcherng@gmail.com>
Signed-off-by: Jack Cherng <jfcherng@gmail.com>
Signed-off-by: Jack Cherng <jfcherng@gmail.com>
@predragnikolic
Copy link
Member

I tested this PR on Linux, and I got an error:

lsp-css: internal/modules/cjs/loader.js:960
lsp-css:   throw err;
lsp-css:   ^

I will investigate further.

@predragnikolic
Copy link
Member

predragnikolic commented Jun 14, 2020

Hmm, I deleted the cache LSP-css folder, and now it works.
Edit: It was my mistake that LSP-css didn't initialize.

@predragnikolic predragnikolic merged commit 70b3661 into sublimelsp:master Jun 14, 2020
@jfcherng jfcherng deleted the update-main-js-file branch June 14, 2020 20:49
@rwols
Copy link
Member

rwols commented Jun 14, 2020

I also noticed this loader error when trying to update LSP-eslint. IMO it should be investigated because I feel like the upgrade mechanism is broken.

@predragnikolic
Copy link
Member

I zipped the broken cache LSP-html folder that throws the error.
And I zipped the good cache LSP-html folder (after I first deleted the broken chacke, and let the plugin install everything from scratch)

I probably only need to find the differences between those folders to see what might be the cause.

Broken-Cache-LSP-html.zip
Good-Cache-LSP-html.zip

@jfcherng
Copy link
Collaborator Author

jfcherng commented Jun 14, 2020

image

The only difference is that there is no out/ at all in the broken one.

rchl pushed a commit to sublimelsp/LSP-html that referenced this pull request Jun 15, 2020
sublimelsp/LSP-css#13

Signed-off-by: Jack Cherng <jfcherng@gmail.com>
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.

4 participants