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

chore: use experimentalExposeScriptSetupContext for vue-tsc #1170

Conversation

cexbrayat
Copy link
Member

This new option allows a better typechecking of components that expose variables.
See vuejs/language-tools#805

Refs #972 as this partially fixes it

@cexbrayat
Copy link
Member Author

@johnsoncodehk I gave a try to the option, as we have a test file expose.spec.ts in the codebase that was not type checking previously (and was excluded form what vue-tsc checks).

Using the option, 2 errors went away 👍

We still have one unexpected error, for a component defined with defineComponent and exposing its variable with expose: in that case, vue-tsc still thinks wrapper.vm does not have access to the property:

tests/expose.spec.ts:28:23 - error TS2339: Property 'other' does not exist on type '{ $: ComponentInternalInstance; $data: {}; $props: Partial<{}> & Omit<Readonly<{} & {} & {}> & VNodeProps & AllowedComponentProps & ComponentCustomProps, never>; ... 10 more ...; $watch(source: string | Function, cb: Function, options?: WatchOptions<...> | undefined): WatchStopHandle; } & ... 4 more ... & ComponentC...'.

28     expect(wrapper.vm.other).toBe('other')

It also raises an error that was not existing before in emit.spec.ts, where we have [EmitsEventSFC, EmitsEventScriptSetup] as DefineComponent[]:

tests/emit.spec.ts:349:11 - error TS2352: Conversion of type 'DefineComponent<{}, { click: () => void; }, {}, {}, {}, ComponentOptionsMixin, ComponentOptionsMixin, "bar"[], "bar", PublicProps, Readonly<...> & { ...; }, {}>[]' to type 'DefineComponent<{}, {}, {}, ComputedOptions, MethodOptions, ComponentOptionsMixin, ComponentOptionsMixin, ... 4 more ..., {}>[]' may be a mistake because neither type sufficiently overlaps with the other. If this was intentional, convert the expression to 'unknown' first.

349   it.each([EmitsEventSFC, EmitsEventScriptSetup] as DefineComponent[])(

We can workaround all this, but maybe that should be fixed in vue-tsc?

@@ -28,7 +28,7 @@ describe('expose', () => {
expect(wrapper.vm.other).toBe('other')
// can't access `msg` as it is not exposed
// and we are in a component with a setup returning a render function
expect(wrapper.vm.msg).toBeUndefined()
expect((wrapper.vm as unknown as { msg: undefined }).msg).toBeUndefined()
Copy link
Member

Choose a reason for hiding this comment

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

Should we be ts-ignoring these or is there more work to come (maybe in Volar) before merging this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is expected to not compile without as, as the test is just to check it is not exposed 😉 So I think it's fine to explicitly cast it to make TS happy.

@cexbrayat
Copy link
Member Author

@johnsoncodehk mentioned on Discord that the first part of the issue is an upstream one vuejs/core#4397 (comment)

I think I'll rewrite the test to get rid of the second one, and maybe ts-ignore the first error while we wait for the issue resolution in vue-next

@cexbrayat cexbrayat force-pushed the chore/experimental-expose-script-setup-context branch from 18121bb to e69e76b Compare December 24, 2021 09:24
This new option allows a better typechecking of components that expose variables.
See vuejs/language-tools#805

Refs vuejs#972 as this partially fixes it
@cexbrayat cexbrayat force-pushed the chore/experimental-expose-script-setup-context branch from e69e76b to eeff860 Compare December 24, 2021 09:26
@cexbrayat
Copy link
Member Author

Done. At the price of one ts-ignore due to the upstream issue, vue-tsc is now capable of type-checking all our tests and components. I also had to rewrite a test to split it in 2, but I don't think this is big deal.

@cexbrayat cexbrayat merged commit eeff860 into vuejs:master Dec 24, 2021
@cexbrayat cexbrayat deleted the chore/experimental-expose-script-setup-context branch December 24, 2021 09:32
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.

2 participants