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

Use vue-typescript (from Volar) for supporting vue, instead of directly using sfc compiler #55

Closed
Shayan-To opened this issue Jun 15, 2022 · 17 comments · Fixed by #56
Closed

Comments

@Shayan-To
Copy link

Shayan-To commented Jun 15, 2022

I was looking for a solution for #40, and I asked in the Volar Discord channel. and the main developer told me that:

johnsoncodehk — 06/13/2022
I see, they could consider using vue-typescript instead
So they do not need to handle SFC parsing and edit range mapping.

Shayan.To — 06/13/2022
Does vue-typescript also support vue 2?

johnsoncodehk — 06/13/2022
If just for organize imports, yes

So it seems like you can entrust the work of parsing and modifying Vue file to vue-tsc project.

@simonhaenisch
Copy link
Owner

Thanks for the issue, however I'm not sure how to use that. The vue-tsc readme is pretty sparse and only says that it's for type-checking vue files, which is not what we would need here (we need a parser). Also looks like there's no proper programmatic API exposed (just a binary).

Did you actually mean using vue-typescript?That's what your chat is talking about at least. I've skimmed through the source (cause there's no readme) and not really sure what to do with that either... really open to a PR for this but I don't currently have any spare time for it.

I've checked #41 again, and reading through the comments I'm just not sure how to go about Vue support in general. There's these edge cases like removal of unused component imports even though they are used in the template, and without a holistic understanding and parsing of the whole Vue file it'll be difficult to get this right.

TBH moving forward I think the best option might be to implement an option for #37 to only sort but not organize imports, and then enable that by default for Vue (so that no imports get removed automatically). Then just need to add the setup script support from the PR.

@Shayan-To
Copy link
Author

Shayan-To commented Jun 15, 2022

You are right, I meant vue-typescript. I was in the middle of something writing this, and I got confused.

If vue-typescript can be a near-drop-in replacement of typescript, wouldn't it be easier to use it for Vue and forget about any issues related to Vue (as they will be forwarded to Volar project)?

(#37 is not particularly useful to me, as I want my unused imports to be actually removed. But I can see that it is indeed a useful feature.)

@simonhaenisch
Copy link
Owner

Ok, so you're saying I can create a language service with createLanguageService from vue-typescript instead of typescript and call organizeImports on it and it'll work? I briefly checked the source of vue-typescript earlier and not sure that actually works, but ok I'll give it a try.

@simonhaenisch
Copy link
Owner

Just tried and I get

Property 'createLanguageService' does not exist on type 'typeof import("[...]/prettier-plugin-organize-imports/node_modules/vue-typescript/lib/index")'.

So vue-typescript is definitely not a drop-in replacement for typescript 🤷🏻‍♂️ (it only has a few exports and none of them seem to be useful here).

@johnsoncodehk
Copy link
Contributor

johnsoncodehk commented Jun 16, 2022

Hi @simonhaenisch, vue-typescript is a wrapper of typescript.createLanguageService, you can use createLanguageServiceContext(require('typescript/lib/tsserverlibrary') /* or require('typescript') */, __LsHostThatIncludingVueFiles__).typescriptLanguageService to get the typescript language service instance.

@Shayan-To Shayan-To changed the title Use vue-tsc (from Volar) for supporting vue, instead of directly using sfc compiler Use vue-typescript (from Volar) for supporting vue, instead of directly using sfc compiler Jun 16, 2022
@simonhaenisch
Copy link
Owner

Hey @johnsoncodehk thanks for the info... I figured out now that I was importing from vue-typescript package not @volar/vue-typescript 🙄 Would be nice if the correct package names were mentioned here, since i have zero context of volar.

Anyway, my existing language service host instance is incompatible because it's missing a getVueCompilationSettings method. Will it be sufficient to add a dummy implementation

getVueCompilationSettings() {
  return {};
}

or where would i get the compilation settings from? I mean this type-checks but not sure it makes any sense.

@johnsoncodehk
Copy link
Contributor

It's here: https://github.com/johnsoncodehk/volar/blob/4e6f6f77c589e39c1139a323fa75a7633eceadc4/packages/vue-typescript/src/types.ts#L9-L22

But getVueCompilationSettings() { return {} } is good enough for use the organize imports api.

@simonhaenisch
Copy link
Owner

I've made a PR but it's failing though... tried running it on a new file, and getting an error Could not find source file "test/file.vue". at getValidSourceFile (.../node_modules/typescript/lib/tsserverlibrary.js). @johnsoncodehk do you know if there's maybe a method I need to implement in my language service host? I'm not too familiar with this, I just implemented the bare minimum to get organizeImports working.

@simonhaenisch
Copy link
Owner

simonhaenisch commented Jun 16, 2022

Hm ok i've had this same problem before and looks like it can be fixed by appending .ts to the filename 🤷🏻‍♂️ I had that in the code before but was assuming that your language service can deal with .vue as well. maybe i'm just missing something.

@johnsoncodehk
Copy link
Contributor

I've made a PR but it's failing though... tried running it on a new file, and getting an error Could not find source file "test/file.vue". at getValidSourceFile (.../node_modules/typescript/lib/tsserverlibrary.js). @johnsoncodehk do you know if there's maybe a method I need to implement in my language service host? I'm not too familiar with this, I just implemented the bare minimum to get organizeImports working.

I can't get your error, it should failed with sorting import properties for me. And if I change compile, defineComponent to defineComponent, compile all tests is passed.

Johnsonde-MacBook-Pro:prettier-plugin-organize-imports johnsonchu$ npm run test

> [email protected] test
> ava --verbose


  ✔ [typescript] sorts imports (734ms)
  ✔ [babel] sorts imports (471ms)
  ✔ [babel-ts] sorts imports (455ms)
  ✔ [typescript] removes partially unused imports (446ms)
  ✔ [babel] removes partially unused imports (439ms)
  ✔ [babel-ts] removes partially unused imports (432ms)
  ✔ [typescript] removes completely unused imports (427ms)
  ✔ [babel] removes completely unused imports (424ms)
  ✔ [babel-ts] removes completely unused imports (421ms)
  ✔ [typescript] works with multi-line imports (416ms)
  ✔ [babel] works with multi-line imports (403ms)
  ✔ [babel-ts] works with multi-line imports (395ms)
  ✔ [typescript] works without a filepath (388ms)
  ✔ [babel] works without a filepath (380ms)
  ✔ [babel-ts] works without a filepath (375ms)
  ✔ [typescript] files with `// organize-imports-ignore` are skipped (370ms)
  ✔ [babel] files with `// organize-imports-ignore` are skipped (366ms)
  ✔ [babel-ts] files with `// organize-imports-ignore` are skipped (363ms)
  ✖ works with TypeScript code inside Vue files 
  ✔ preserves new lines and comments in Vue files
  ─

  works with TypeScript code inside Vue files

  test.js:113

   112:                                                                                         
   113:   t.is(formattedCode.split('\n')[1], `import { compile, defineComponent } from "vue";`);
   114: });                                                                                     

  Difference:

  - 'import { defineComponent, compile } from "vue";'
  + 'import { compile, defineComponent } from "vue";'

  › test.js:113:4

  ─

  1 test failed

Hm ok i've had this same problem before and looks like it can be fixed by appending .ts to the filename 🤷🏻‍♂️ I had that in the code before but was assuming that your language service can deal with .vue as well. maybe i'm just missing something.

Yes it can handle .vue. But not sure why not works for you yet, the cicd has same result with me: https://github.com/simonhaenisch/prettier-plugin-organize-imports/runs/6915996161?check_suite_focus=true

@simonhaenisch
Copy link
Owner

You can only see the error if you run it like DEBUG=true npm test because the plugin doesn't print errors by default (so that it doesn't mess up prettier's output).

And if I change compile, defineComponent to defineComponent, compile all tests is passed.

Yeah but then that would mean that the plugin didn't work as expected (because it sorts imports alphabetically) 🙃

Not sure why it doesn't work with .vue here but if i change it to .vue.ts then everything works as expected now. 🤷🏻‍♂️ might just be missing something with how i'm creating the language service host.

Anyway, thanks for your help!

@simonhaenisch
Copy link
Owner

@Shayan-To can you please try out version 3.0.0

@johnsoncodehk
Copy link
Contributor

@simonhaenisch It seems @volar/vue-typescript issue, I will investigate with it later. (You can open an issue in https://github.com/johnsoncodehk/volar/issues if you want)

@Shayan-To
Copy link
Author

@simonhaenisch @johnsoncodehk This is an unexpected change:

@@ -61,22 +61,20 @@
         </SimpleDialog>
     </div>
 </template>
 
 <script lang="ts">
-import { defineComponent } from "vue";
-export default defineComponent({ inheritAttrs: false });
-</script>
-
-<script setup lang="ts">
 import { TrackerDefinition, trackerDefinitions } from "src/data/trackers";
 import { getObjectValues } from "src/utils";
 import { imUsing } from "src/utils/vue";
 import SimpleDialog from "src/view/components/SimpleDialog.vue";
-import { computed, Ref, toRefs } from "vue";
+import { computed, defineComponent, Ref, toRefs } from "vue";
 import { SetupTrackerPages } from "./setup-tracker-pages";
+export default defineComponent({ inheritAttrs: false });
+</script>
 
+<script setup lang="ts">
 imUsing(trackerDefinitions, SimpleDialog, getObjectValues);
 
 const props = defineProps<{
     selectedTracker: Ref<TrackerDefinition | null>;
     currentPage: Ref<SetupTrackerPages>;

I also found two more issues:

  • When prettier is run from VSCode, it behaves differently from when it's run from the command line. In VSCode, it doesn't touch the imports, and only formats the document. (I the behavior is only for .vue files. It doesn't behave like this for .ts files.)

  • When run from the CLI, the imports that are only used in the template are removed. (I observed this behavior for imported components, directives, and functions.)

@simonhaenisch
Copy link
Owner

Well, at this point I'm not sure what to do about this 🤷🏻‍♂️ because now it's up to @volar/vue-typescript what actually happens when organizeImports is called, because we're not using the plain TypeScript language service API anymore for Vue. So I guess you'll have to raise that issue with Volar.

Regarding the difference between Prettier from VS Code vs CLI, I'm really not sure how that's possible, but I'm assuming the VS Code plugin has some special logic to run Prettier on file ranges based on their language, i. e. it would run in typescript language mode for a section of the document that it detects as TypeScript. Afaik it can do that e. g. for fenced code blocks in markdown, so maybe it does the same for script blocks in Vue files?

Or maybe you have set some of the prettier options in your VS Code config that are changing something?

@Shayan-To
Copy link
Author

@johnsoncodehk Should I open an issue on Volar repo? Should I provide a repro link? (I didn't provide a repro because it did the same on almost all my files.)

@johnsoncodehk
Copy link
Contributor

@Shayan-To Yes please open an issue to Volar repo, repro link is not needed, but please describe current behavior and expected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants