sublimelsp / LSP-angular

Convenience plugin for Angular Language Service
MIT License
9 stars 2 forks source link

Bump to Angular 16 #97

Closed princemaple closed 1 year ago

princemaple commented 1 year ago

close #96

princemaple commented 1 year ago

Can I get maintainer access to this repo? @predragnikolic @rchl

predragnikolic commented 1 year ago

@princemaple yes, I will give you access tomorrow

rchl commented 1 year ago

Will this version only be compatible with projects that use Angular 16 or also the previous versions? Or does the server use dependencies from the project?

predragnikolic commented 1 year ago

@princemaple I've send you a invitation to be a maintainer for this repo.

Thanks for the initiative!

princemaple commented 1 year ago

@rchl This most likely destroys it for previous versions.

Ideally we should allow configuration for these variables https://github.com/sublimelsp/LSP-angular/blob/master/LSP-angular.sublime-settings#L3C35-L7 so that the language server can be started from the project local deps directly.

princemaple commented 1 year ago

Without the said configuration, basically everyone who's not on the current version that lsp-angular is on needs to do this workaround https://github.com/sublimelsp/LSP-angular/issues/96.

Maybe there was some level of compatibility before v16, because ngcc is removed in v16. 🤔

Anyway. We need to be able to configure this and run local deps, @angular/language-service and typescript.

princemaple commented 1 year ago

@rchl Any guidance on whether/how that can be achieved? 😄

predragnikolic commented 1 year ago

I would say that users could override the command (preferably in a ST project file) to point to a locally installed angular lsp server.

Maybe we should just give an example in the readme on how to do that.

https://github.com/sublimelsp/LSP-angular/blob/706a02996949ab923fb777562ca6f7f23b71e52c/LSP-angular.sublime-settings#L3

princemaple commented 1 year ago

Yeah an example of overwriting the whole command might just do the job.

rchl commented 1 year ago

But do projects actually include the @angular/language-server dependency normally? Or only @angular/language-service (server vs. service)?

I don't know enough about angular to give any advice here. I suppose I would try to see what the VSCode extension does in this case.

I saw https://github.com/sublimelsp/LSP-angular/issues/96#issuecomment-1537545676 but I'm not sure the quoted comment really means what you thought there @predragnikolic. They only seem to talk about compatiblity between typescript and the service. I don't see any mention of using new version in a project that doesn't use newest angular.

princemaple commented 1 year ago

Angular projects are generated with @angular/language-service but not @angular/language-server. In case where a user is using a incompatible version, we'd like to support their self-installed server.

Actually another option came into mind. Maybe we can have

And allow users to pick one? default to the latest. So they don't have to install a separate server, and we re-use most of our existing logic, only changing a path.

princemaple commented 1 year ago

What do you think @predragnikolic ? I think this would be the easiest to use.

Actually another option came into mind. Maybe we can have

* `server-16/`
* `server-15/`
* `server-14/`
* etc

Have respective package.json and package-lock.json in them.

Now, if I understood the code correctly, we just need to read an angular version from the settings and change this variable https://github.com/sublimelsp/LSP-angular/blob/master/plugin.py#L15 and we will be able to handle any version of angular (given that we have the respective folder of package.json)

predragnikolic commented 1 year ago

I can finally focus on this 🙂

I will try a couple of approaches(hopefully tomorrow), and share my conclusions. (I don't want to rush this decision)

princemaple commented 1 year ago

😄 take all the time you need.

predragnikolic commented 1 year ago

Hello, I would not add additional settings to this package to select the Angular version. I don't want to add that responsibility here.

Let this package always ship the latest server. (We will also probably add a sublime-package.json file in the future, to improve the settings auto completion)

For people who need an older version of the angular server, I would put in the README a section, on how to point to a local @angular/language-server.

They can override the start command in a ST project file, to point to a local @angular/language-server

{
    "folders":
    [
        {
            "path": "."
        }
    ],
    "settings": {
        "LSP": {
            "LSP-angular": {
                "command": [
                    "${node_bin}",
                    "${folder}/node_modules/@angular/language-server/index.js", // maybe adjust the path based on your project
                    "--ngProbeLocations", "${folder}/node_modules",  // maybe adjust the path based on your project
                    "--tsProbeLocations", "${folder}/node_modules",  // maybe adjust the path based on your project
                    "--stdio"
                ]
            },
        }
    }
}

They just need to make sure that the @angular/language-server lib is compatible with the typescript version in the project.

I experienced a typescript mismatch right away :D because I installed a @angular/language-server that required an older version of typescript... I noticed the error immediately in the LSP log panel:

LSP-angular: /home/predragnikolic/Documents/sandbox/ang-app/node_modules/@angular/language-server/index.js:157
LSP-angular:       throw new Error(`Failed to resolve '${packageName}' with minimum version '${minVersion}' from ` + JSON.stringify(probeLocations, null, 2));
LSP-angular:       ^
LSP-angular: 
LSP-angular: Error: Failed to resolve 'typescript/lib/tsserverlibrary' with minimum version '4.2' from []

Other people will probably not notice this error, They will just see The LSP-angular server crached 5 times in 180 sec. and probably won't know what is wrong. So they need to make sure that the versions between typescript and @angular/language-server are compatible, if the server fails to start, the best would be to check the log panel for additional info.

predragnikolic commented 1 year ago

What do you guys think?

princemaple commented 1 year ago

They just need to make sure that the @angular/language-server lib is compatible with the typescript version in the project.

This is the reason why I suggested preparing the package.json files for different angular versions, so that the users wouldn't have to do anything but specifying the angular version.

server-16/
|-- package.json - specifies angular 16 and ts 5.0
|-- package-lock.json - npm generated
server-15/
|-- package.json - specifies angular 15 and ts 4.8
|-- package-lock.json - npm generated
...
...
...

And read a setting (which defaults to the latest angular version, 16 for now) to dyanmically set this variable https://github.com/sublimelsp/LSP-angular/blob/master/plugin.py#L15

If this is not desired, then I guess we just need to merge this PR as is and add some README instructions.

This really doesn't affect me, as I'm always on the latest version. 😄

princemaple commented 1 year ago

Actually do we even need typescript in the package.json? We should be able to directly use the typescript installed in the user's project, it has to exist as it's an angular project.

It is probably required because it's harder to locate the user's ts if it's in a mono repo.

Klaster1 commented 1 year ago

Any updates?

princemaple commented 1 year ago

Hi @Klaster1 are you waiting on v16 update?

Klaster1 commented 1 year ago

Well, I already worked around the issue by installing TS 5 and language server 16 into a separate dir and changing the LSP-angular command, but I figured it would be OK to nudge the PR a bit 😉

{
    "command": [
        "${node_bin}",
        "c:\\dev\\ggstuff\\language-server\\node_modules\\@angular\\language-server\\index.js",
        "--logFile",
        "c:\\dev\\ggstuff\\language-server\\ngls.log",
        "--ngProbeLocations",
        "c:\\dev\\ggstuff\\language-server\\node_modules\\@angular\\language-server\\node_modules",
        "--tsProbeLocations",
        "c:\\dev\\ggstuff\\language-server\\node_modules\\@angular\\language-server\\node_modules",
        "--stdio"
    ]
}

As to the 15/16 language server approach, I think the readme solution is good enough since I had a similar issue with LSP-typescript and solved it the same way.