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: bump to vue v3.2.34 #1510

Merged
merged 5 commits into from May 20, 2022
Merged

Conversation

cexbrayat
Copy link
Member

This is a repro to showcase the regressions we have with vue v3.2.34 (currently beta.1)

As you can see in the build, we are running into some TS issues:

FAIL  tests/features/compat.spec.ts
  ● Test suite failed to run

    tests/features/compat.spec.ts:176:23 - error TS2339: Property 'foo' does not exist on type 'ComponentPublicInstance<unknown, {}, {}, {}, {}, {}, unknown, {}, false, ComponentOptionsBase<any, any, any, any, any, any, any, any, any, {}, ComponentProvideOptions>>'.

    176     expect(wrapper.vm.foo).toBe('bar')
                              ~~~

 FAIL  tests/mountingOptions/mocks.spec.ts
  ● Test suite failed to run

    tests/mountingOptions/mocks.spec.ts:50:44 - error TS2339: Property 'id' does not exist on type 'CreateComponentPublicInstance<{ [x: string & `on${string}`]: ((...args: never) => any) | undefined; } | { [x: string & `on${string}`]: ((...args: any[]) => any) | undefined; }, {}, {}, {}, {}, ComponentOptionsMixin, ... 12 more ..., ComponentProvideOptions>'.
      Property 'id' does not exist on type '{ $: ComponentInternalInstance; $data: {}; $props: { [x: string & `on${string}`]: ((...args: never) => any) | undefined; } | { [x: string & `on${string}`]: ((...args: any[]) => any) | undefined; }; ... 10 more ...; $watch(source: string | Function, cb: Function, options?: WatchOptions<...> | undefined): WatchStopHan...'.

    50           this.$router.push(`/posts/${this.id}`)

The PR also showcases why we need vuejs/core#5947
and a weird TS inference problem

@cexbrayat cexbrayat requested a review from pikax May 18, 2022 10:21
@netlify
Copy link

netlify bot commented May 18, 2022

Deploy Preview for vue-test-utils-docs ready!

Name Link
🔨 Latest commit cc35833
🔍 Latest deploy log https://app.netlify.com/sites/vue-test-utils-docs/deploys/6285eeff8b53ff00081cb0ee
😎 Deploy Preview https://deploy-preview-1510--vue-test-utils-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@cexbrayat
Copy link
Member Author

@pikax With your fix and mine, I was able to remove the workarounds to update to 3.2.34
But we still have the type-checking errors in tests. Any ideas?

 FAIL  tests/features/compat.spec.ts
  ● Test suite failed to run

    tests/features/compat.spec.ts:176:23 - error TS2339: Property 'foo' does not exist on type 'ComponentPublicInstance<unknown, {}, {}, {}, {}, {}, unknown, {}, false, ComponentOptionsBase<any, any, any, any, any, any, any, any, any, {}, ComponentProvideOptions>>'.

    176     expect(wrapper.vm.foo).toBe('bar')
                              ~~~

 FAIL  tests/mountingOptions/mocks.spec.ts
  ● Test suite failed to run

    tests/mountingOptions/mocks.spec.ts:50:44 - error TS2339: Property 'id' does not exist on type 'CreateComponentPublicInstance<{ [x: string & `on${string}`]: ((...args: never) => any) | undefined; } | { [x: string & `on${string}`]: ((...args: any[]) => any) | undefined; }, {}, {}, {}, {}, ComponentOptionsMixin, ... 12 more ..., ComponentProvideOptions>'.
      Property 'id' does not exist on type '{ $: ComponentInternalInstance; $data: {}; $props: { [x: string & `on${string}`]: ((...args: never) => any) | undefined; } | { [x: string & `on${string}`]: ((...args: any[]) => any) | undefined; }; ... 10 more ...; $watch(source: string | Function, cb: Fun
chore: bump to vue v3.2.34
ction, options?: WatchOptions<...> | undefined): WatchStopHan...'.

    50           this.$router.push(`/posts/${this.id}`)

@cexbrayat cexbrayat changed the title chore: bump to vue v3.2.34-beta.1 chore: bump to vue v3.2.34 May 19, 2022
Comment on lines 119 to 121
// Find my CatchAll component
findComponent<T extends Component>(selector: T): DOMWrapper<Node>
findComponent<T extends Component>(selector: string): DOMWrapper<Element>
Copy link
Member

Choose a reason for hiding this comment

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

This is a more generic way to catch components, I've removed the other findComponent overloads and the tests still passed, still not sure if removing the other overloads would cause an issue

Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't these methods return a VueWrapper<T>?

Copy link
Member

Choose a reason for hiding this comment

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

Yes 🤦

Comment on lines -41 to 45
url() {
url(): string {
return `/posts/${this.$route.params.id}`
},
id() {
id(): string | string[] {
return this.$route.params.id
Copy link
Member

Choose a reason for hiding this comment

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

Concerned about this change, not 100% sure what's happening here

Copy link
Member

Choose a reason for hiding this comment

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

route.params[key] is typed like this

image

What is your concern?

Copy link
Member Author

Choose a reason for hiding this comment

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

The concern is that we shouldn't have to explicitly specify the type: the inference is somehow broken with V3.2.34

Copy link
Member

Choose a reason for hiding this comment

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

Having to annotate the return type is something is there since V2, but this is not an issue with Vue test utils, is more a concern with Vue 3.2.34 release, all good on test utils IMO

@cexbrayat cexbrayat marked this pull request as ready for review May 19, 2022 07:21
@lmiller1990 lmiller1990 merged commit e59981d into vuejs:main May 20, 2022
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.

None yet

3 participants