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

fix(vue): vue regular script block exports not being recognized inside editor #8998

Merged

Conversation

minht11
Copy link
Contributor

@minht11 minht11 commented Nov 4, 2023

Closes #8947

Changes

Adds editor support/fixes non setup vue block export types inside astro components.
Also add support for Vue 3.3 generics, when explicitely passing types to defineProps.

Previously import types from first block

<script lang="ts">
export const myFn = (param: 1) => param

export type MyType = 1
</script>

<script lang="ts" setup>
defineProps<{ prop: number }>()
</script>

<template>
  <div></div>
</template>

inside astro components would not be recognized

---
import { type MyType, myFn } from '../components/Foo.vue'
---

The reason being because first block source code was not included inside editor plugin generated virtual file.

Testing

I locally patched /node_modules/@astrojs/vue/dist/editor.cjs and after editor restart types started to work.

I did not find any tests for vue editor integration in this repo so I didn't add any here either.

Docs

This is a minor fix.


I wasn't sure if this is minor or patch, because while runtime supports it, editor never did, so it is almost a new feature. Feel free to correct me.

Copy link

changeset-bot bot commented Nov 4, 2023

🦋 Changeset detected

Latest commit: b450334

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: vue Related to Vue (scope) pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review labels Nov 4, 2023
Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

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

Would just suggest our present tense, but otherwise LGTM!

.changeset/two-oranges-knock.md Outdated Show resolved Hide resolved
@sarah11918 sarah11918 removed the pr: docs A PR that includes documentation for review label Nov 6, 2023
@github-actions github-actions bot added the pr: docs A PR that includes documentation for review label Nov 6, 2023
@ematipico
Copy link
Member

I don't know Vue very much, but considering that you're adding a new feature, maybe should create new tests that shows that we support it.

We have fixtures here, where we emulate real world projects: https://github.com/withastro/astro/tree/main/packages/integrations/vue/test/fixtures

And here you can add a new test. Here's an example of an existing test: https://github.com/withastro/astro/blob/main/packages/integrations/vue/test/app-entrypoint.test.js

Plus, while we a fixing a bug, we are actually adding support for something new, so we should document it.

@minht11
Copy link
Contributor Author

minht11 commented Nov 6, 2023

@ematipico are you talking about testing types inside editor or runtime support? Runtime already works, I can add few more tests which cover different vue component shapes with multiple blocks and generics. Not sure how to test types though.

@ematipico
Copy link
Member

Yeah, for types, we can't test it; the only thing we can do - I think:

  • create a component with types and... assert something, as long as they compile, they work, I suppose;
  • maybe we can update the examples/ folder;
  • Should we update the documentation?

@minht11
Copy link
Contributor Author

minht11 commented Nov 6, 2023

I added few more tests, I took inspiration from react integrations tests, let me know if I need to add more.

About examples. As far as I looked every example there is very simple, no other framework has any special concepts besides counter added. Adding generics there would complicate things and I am not sure we want that? If needed to add example I guess I could add something similar to GenericsAndBlocks.vue test.

Which leads me to documentation, these features are just basic vue features, in react guide I don't see anything added about Typescript generics or that you can export multiple things from the same component file. Those things seem better left to the framework?

I am happy to add more examples/documentation just not sure where those would that fit given current structure.

@Princesseuh
Copy link
Member

imo this PR doesn't require tests in this repo. Since it's ultimately our language server (at https://github.com/withastro/language-tools) which consume those modules, testing it there seems more appropriate to me (especially since all the testing tooling is already there to test the types)

<div></div>
}
`;
} else {
const defineProps =
parsedResult.descriptor.scriptSetup.content.match(/defineProps\([\s\S]+\)/m);
// TODO. Find a way to support generics when using defineProps without passing explicit types.
Copy link
Member

Choose a reason for hiding this comment

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

Ultimately, this module shouldn't use a regex to get the types, it should use something like https://github.com/vuejs/language-tools/tree/master/packages/component-meta and https://github.com/vuejs/language-tools/tree/master/packages/component-type-helpers

which should support everything (or at least, if it doesn't, would be a bug upstream that's not in our hands)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, I have looked briefly into these packages after I opened this PR and its not so far I haven't found an easy way.

component-meta gives narrows prop types and output loses generic information, so T extends string becomes just string.

component-type-helpers needs a component instance types, to import which requires vue language server I think and still it loses all generic information and narrows types. I am not sure how would that work with virtual file.

Current approach is not sustainable and prone to fail, right now astro doesn't account for https://vuejs.org/guide/typescript/composition-api.html#props-default-values or experimental defineModel and so on and yet I am not sure how to get something better in a short term.

Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Copy link
Member

@Princesseuh Princesseuh left a comment

Choose a reason for hiding this comment

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

Looks great, thank you! In the future we'll find a real solution to not use a regex, but honestly, it (mostly) works and that's already pretty good.

@Princesseuh Princesseuh merged commit 14e586c into withastro:main Nov 8, 2023
13 checks passed
@astrobot-houston astrobot-houston mentioned this pull request Nov 8, 2023
@minht11 minht11 deleted the fix/vue-types-with-multiple-script-blocks branch November 8, 2023 13:54
natemoo-re pushed a commit that referenced this pull request Nov 22, 2023
…e editor (#8998)

Co-authored-by: Erika <3019731+Princesseuh@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope) pkg: vue Related to Vue (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Astro does not support Vue exports other than from setup.
4 participants