Vue Support - #83

[zulus]

I wasn't sure howto correctly register other language files defined in https://github.com/vuejs/language-tools/blob/master/packages/vscode-vue/package.json

*.java classes are mostly copy-paste from typescript server

[zulus]

what now?

[vrubezhny]

what now?

@zulus Thanks for contributing!

It would be great if you add at least some simple JUnit tests with vue.js cases (like to see if vue.js specific diagnostic/code assist/etc. is provided by the LS),

[vrubezhny]

/request-license-review

[github-actions]

/request-license-review

⚠️ Failed to request review of not vetted licenses.

Workflow run (with attached summary files):
https://github.com/eclipse/wildwebdeveloper/actions/runs/5237919073

[vrubezhny]

/request-license-review

[github-actions]

/request-license-review

⚠️ Failed to request review of not vetted licenses.

Workflow run (with attached summary files):
https://github.com/eclipse/wildwebdeveloper/actions/runs/5237945044

[vrubezhny]

The IP Team Review requests are created regarding the proposal:

• npm/npmjs/@vue/language-core/1.7.0
• npm/npmjs/-/volar-service-emmet/0.0.2
• npm/npmjs/-/volar-service-html/0.0.2
• npm/npmjs/@vscode/emmet-helper/2.8.9
• npm/npmjs/-/typesafe-path/0.2.2
• npm/npmjs/-/volar-service-css/0.0.2
• npm/npmjs/-/volar-service-pug/0.0.2
• npm/npmjs/-/volar-service-typescript/0.0.2
• npm/npmjs/-/volar-service-typescript-twoslash-queries/0.0.2
• npm/npmjs/@johnsoncodehk/pug-beautify/0.2.2
• npm/npmjs/-/volar-service-json/0.0.2
• npm/npmjs/@vue/language-service/1.7.0
• npm/npmjs/@babel/parser/7.22.5
• npm/npmjs/-/volar-service-pug-beautify/0.0.2
• npm/npmjs/@vue/language-server/1.7.0

[zulus]

what now?

@zulus Thanks for contributing!

It would be great if you add at least some simple JUnit tests with vue.js cases (like to see if vue.js specific diagnostic/code assist/etc. is provided by the LS),

Sure!

[zulus]

I prepared basic tests, similar to angular:

1. Test marker inside method
2. Test marker inside template
3. Completion proposal based on component javascript source
4. Completion proposal based on broken template

[zulus]

BTW: during playing with tests, I realized that TestAngular and TestESLint might use system node, rather than built-in

[vrubezhny]

BTW: during playing with tests, I realized that TestAngular and TestESLint might use system node, rather than built-in

Why do you think so? They both use NodeJSManager.getNpmLocation().getAbsolutePath() to get an npm executable location, then add arguments for options.

In case of your test, you're generating a command that runs Node.js executable, which gets a path to NPM executable as first argument, then arguments for options...

[D:\a\wildwebdeveloper\wildwebdeveloper\org.eclipse.wildwebdeveloper.tests\target\work\.node\node-v18.16.0-win-x64\node.exe, D:\a\wildwebdeveloper\wildwebdeveloper\org.eclipse.wildwebdeveloper.tests\target\work\.node\node-v18.16.0-win-x64\npm.cmd, install, --no-bin-links, --ignore-scripts

not sure how this supposed to work...

[zulus]

BTW: during playing with tests, I realized that TestAngular and TestESLint might use system node, rather than built-in

Why do you think so? They both use NodeJSManager.getNpmLocation().getAbsolutePath() to get an npm executable location, then add arguments for options.

In case of your test, you're generating a command that runs Node.js executable, which gets a path to NPM executable as first argument, then arguments for options...

[D:\a\wildwebdeveloper\wildwebdeveloper\org.eclipse.wildwebdeveloper.tests\target\work\.node\node-v18.16.0-win-x64\node.exe, D:\a\wildwebdeveloper\wildwebdeveloper\org.eclipse.wildwebdeveloper.tests\target\work\.node\node-v18.16.0-win-x64\npm.cmd, install, --no-bin-links, --ignore-scripts

not sure how this supposed to work...

On mac, this code produces me only path to NPM location. NPM for *nix have /bin/env node, but tests ignore PATH produced by NodeJSManager

[zulus]

On mac, this code produces me only path to NPM location. NPM for *nix have /bin/env node, but tests ignore PATH produced by NodeJSManager

See https://github.com/eclipse/wildwebdeveloper/blob/e52e0990f609f3dcbac5648a4bd012ecf9d753a5/org.eclipse.wildwebdeveloper.embedder.node/src/org/eclipse/wildwebdeveloper/embedder/node/NodeJSManager.java#L120, on *nix will return true, +x should be removed from npm script files, or just remove "can execute" checking for npm.

This is also reason why windows build not working. So I dropped attaching node path in Vue tests.

[vrubezhny]

, on *nix will return true, +x should be removed from npm script files, or just remove "can execute" checking for npm.

I wonder why nobody else did complain about Mac & Win NPM run failures until now...
Unfortunately I cannot verify in locally on Mac, but previously ran tests for Angular and ESLint stuff successfully execute NPM install using the embedded NPM executable.

If it's true then it's worth reporting it as an issue

But looking at the GitHub/Jenkins check logs I see NPM executable started from

D:\a\wildwebdeveloper\wildwebdeveloper\org.eclipse.wildwebdeveloper.tests\target\work\.node\node-v18.16.0-win-x64\npm.cmd

on Windows and

/home/jenkins/agent/workspace/Wildwebdeveloper_PR-1241/org.eclipse.wildwebdeveloper.tests/target/work/.node/node-v18.16.0-linux-x64/bin/npm

on Linux - which are surely the Embedded versions, not the system ones.
Probably we have to add MacOS to the test OS matrix

[zulus]

Maybe because I've run tests via Junit plugin launcher inside eclipse, not by mvn verify, or other contributors have node inside main PATH (so tests just uses it), my PATH for JUnit Launcher is always fresh ;) I'll prepare ticket.

As I see build fail on mvn-license-check, can I do something?

Npm licenses has been successfully resolved by IP Team as I see.

Any other suggestions for my contribution? I act intuitively now and probably haven't enough experience with LSP4E/TM4E, so just tell me what I should do :D ( this patch is an initiation before I start some PDT improvements :P )

[vrubezhny]

Maybe because I've run tests via Junit plugin launcher inside eclipse, not by mvn verify, or other contributors have node inside main PATH (so tests just uses it), my PATH for JUnit Launcher is always fresh ;) I'll prepare ticket.

That's a question, but, imho, yes the system Node.js/Npm may be used in this case (and that's expected)

As I see build fail on mvn-license-check, can I do something?

Just give me some time to review the PR.

Npm licenses has been successfully resolved by IP Team as I see.

Yes, and that's great.

Any other suggestions for my contribution? I act intuitively now and probably haven't enough experience with LSP4E/TM4E, so just tell me what I should do :D ( this patch is an initiation before I start some PDT improvements :P )

If you have nothing to add to the PR at this moment we can merge it (after review) as is and then improve the code later. Imho, this is a great improvement.

[vrubezhny]

(Sorry, closed unintentionally by clicking the wrong button. Everything is restored and open)

[vrubezhny]

Overall looks good. Please see the requested changes to be fixed before we can merge

[vrubezhny]

@zulus Could you please rebase and resolve the requested changes?

[zulus]

@zulus Could you please rebase and resolve the requested changes?

Done, I updated license headers, added missing one, and rebased to current HEAD (I see license problems now :/)

BTW, in the meantime vue-language server has been upgraded (1.7.0 to 1.8.3 - https://github.com/vuejs/language-tools/blob/master/CHANGELOG.md), so after merge I'll prepare another pull request with new version

[vrubezhny]

/request-license-review

[vrubezhny]

Done, I updated license headers, added missing one, and rebased to current HEAD (I see license problems now :/)

I created the Review requests for the updated modules, hope they will be resolved soon.

BTW, in the meantime vue-language server has been upgraded (1.7.0 to 1.8.3 - https://github.com/vuejs/language-tools/blob/master/CHANGELOG.md), so after merge I'll prepare another pull request with new version

Actually you can add this change right now - it will require an additional review for the LS (as it's minor version is changed), but we haven't had any problems with the IP Team review for the previous version, so I don't expect any with the latest one.

[zulus]

Actually you can add this change right now - it will require an additional review for the LS (as it's minor version is changed), but we haven't had any problems with the IP Team review for the previous version, so I don't expect any with the latest one.

Done

[vrubezhny]

/request-license-review

[vrubezhny]

I've started the IP Team review for Vue.js LS 1.8.3 - so need to wait until it finshes

@zulus please follow #1241 (comment)

[vrubezhny]

The code looks good to me.

@zulus Thank you for the contribution!