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

feat: upgrade volar #124

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

so1ve
Copy link

@so1ve so1ve commented Feb 27, 2024

resolve #98

Copy link

@Shinigami92 Shinigami92 left a comment

Choose a reason for hiding this comment

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

I just checked out the fork with this PR and linked it to a mono-repo using "prettier-plugin-organize-imports": "link:../../../OpenSource/prettier-plugin-organize-imports"

  • used pnpm install
  • reloaded VS Code
  • run prettier

It worked instantly, via CLI and via VSCode format 🤩

@Shinigami92
Copy link

@simonhaenisch I didn't looked into the code, so feel free to make a code review before merging 🙂

@Shinigami92
Copy link

I also didn't need to install npm i --save-dev @vue/language-core 🤷
But maybe because on of the packages inside the monorepo uses Vue and so when I go into that package it shows pnpm why @vue/language-core:

devDependencies:
vite-plugin-dts 3.7.3
├── @vue/language-core 1.8.27
└─┬ vue-tsc 1.8.27
  └── @vue/language-core 1.8.27
vue-tsc 1.8.27
└── @vue/language-core 1.8.27

@Shinigami92
Copy link

@simonhaenisch what's the state? did you have time yet to make a review?

@aphex
Copy link

aphex commented Apr 5, 2024

bump on this one @simonhaenisch looking forward to dropping @volar/vue-typescript from our deps. Not sure there is anything I can do to help push it along, and I do understand "I'm to busy" is a likely answer.

@simonhaenisch
Copy link
Owner

Sorry and thanks for the pings... kinda forgot about it, i'll try to get it done this week.

@so1ve
Copy link
Author

so1ve commented Apr 8, 2024

Volar just released v2, shall we upgrade it?

@simonhaenisch
Copy link
Owner

Yes please, let's get the breaking changes all done at once (:

@so1ve do you have time to update your PR?

@so1ve
Copy link
Author

so1ve commented Apr 10, 2024

Sure, I'll work on it

@Shinigami92
Copy link

Sure, I'll work on it

@so1ve What's the progress?

@so1ve
Copy link
Author

so1ve commented Apr 17, 2024

Sorry, but there are a few bugs that I had to work on, such as inaccurate text edit positions 😓

@Shinigami92
Copy link

Sorry, but there are a few bugs that I had to work on, such as inaccurate text edit positions 😓

Can I help somehow?

@aphex
Copy link

aphex commented May 14, 2024

Checking back in on this one. @so1ve I see some code landed a week ago, though labeled WIP so maybe there is more to be done before this all gets considered? If there is help needed let me know, even if it is just testing. Just would need some steps.

"optional": true
}
},
"devDependencies": {
"@types/node": "20.8.10",
"@volar/vue-language-plugin-pug": "1.0.9",
Copy link
Owner

Choose a reason for hiding this comment

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

Where did this go in the dev dependencies? didn't we have a test case that was using this plugin?

Choose a reason for hiding this comment

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

If pug is not working without it, it should be changed to https://www.npmjs.com/package/@vue/language-plugin-pug cause @volar/vue-language-plugin-pug is deprecated
It might be needed somehow so imports get not lost when using <template lang="pug">

getTypeScriptModule: () => ts,
...ts.sys,
configFileName: undefined,
getCurrentDirectory: () => '/',
Copy link
Owner

Choose a reason for hiding this comment

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

Why /?

getScriptFileNames: () => [filepath],
getScriptSnapshot: (filename) => {
if (filepath === filename) return ts.ScriptSnapshot.fromString(code);
},
Copy link
Owner

Choose a reason for hiding this comment

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

These all seem to be part of the return value of getTypeScriptLanguageServiceHost already, why can't we pick them from there, like before?

@so1ve
Copy link
Author

so1ve commented May 16, 2024

Oh I figured it out it is caused by tabs 😅 For some reasons volar incorrectly calculates tab character's width

@mesqueeb
Copy link

@so1ve @simonhaenisch friendly bump

what are the remaining steps to get this released?

@arkandias
Copy link

Hi there!
Looking forward to seeing this update released. Is there something I could do to help?

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.

Volar packages renamed
6 participants