-
Notifications
You must be signed in to change notification settings - Fork 244
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
Conversation
✅ Deploy Preview for vue-test-utils-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
d06eec7
to
392f08e
Compare
392f08e
to
8f9a91d
Compare
@pikax With your fix and mine, I was able to remove the workarounds to update to 3.2.34
|
src/baseWrapper.ts
Outdated
// Find my CatchAll component | ||
findComponent<T extends Component>(selector: T): DOMWrapper<Node> | ||
findComponent<T extends Component>(selector: string): DOMWrapper<Element> |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes 🤦
url() { | ||
url(): string { | ||
return `/posts/${this.$route.params.id}` | ||
}, | ||
id() { | ||
id(): string | string[] { | ||
return this.$route.params.id |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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:
The PR also showcases why we need vuejs/core#5947
and a weird TS inference problem