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

feat: getComponent #89

Merged
merged 2 commits into from
Apr 24, 2020
Merged

Conversation

cexbrayat
Copy link
Member

This introduces a getComponent which returns a VueWrapper or throws.
It offers two benefits:

  • a symetric API for components/elements with findComponent/findAllComponents/getComponent/find/findAll/get
  • a better experience for TS users (as findComponent returns a union type, you can't chain certain things until you disambiguated the type manually).

I also refactored the signatures of find to offer proper overloads (the current version breaks tests compilation in a real app, i hope this fixes it). I added TSD tests to ensure the inference is working properly.

This introduces a `getComponent` which returns a `VueWrapper` or throws.
It offers two benefits:
- a symetric API for components/elements with `findComponent/findAllComponents/getComponent`/`find/findAll/get`
- a better experience for TS users (as `findComponent` returns a union type, you can't chain certain things until you disambiguated the type manually).

I also refactored the signatures of `find` to offer proper overloads (the current version breaks tests compilation in a real app, i hope this fixes it). I added TSD tests to ensure the inference is working properly.
@lmiller1990
Copy link
Member

lmiller1990 commented Apr 24, 2020

I had this exact thought the other day - I think this makes sense. We have get, after all.

I see you didn't include getAllComponents - this makes sense too, there is no real use case for that.

I will wait for at least one more approval on this PR, to let others have input as well 👍

An aside: testing types is a very cool concept I just learned about recently.

src/vue-wrapper.ts Outdated Show resolved Hide resolved
tests/getComponent.spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@afontcu afontcu left a comment

Choose a reason for hiding this comment

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

Looks good overall, other than the small details I suggested 👍

@cexbrayat
Copy link
Member Author

@lmiller1990 👍 Thank you for the quick review.

@afontcu I pushed a new commit with your suggestion for getComponent, and changed it in get as well.

@lmiller1990 lmiller1990 merged commit a0f87d6 into vuejs:master Apr 24, 2020
@lmiller1990
Copy link
Member

Nice, I will do a new alpha this weekend.

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.

3 participants