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

Find by component #66

Merged
merged 11 commits into from
Apr 18, 2020
Merged

Find by component #66

merged 11 commits into from
Apr 18, 2020

Conversation

dobromir-hristov
Copy link
Contributor

This PR allows finding a Vue Component and returning a new VueWrapper with it's instance.

Caveats:

  • Returns only Vue components, and works only on VueWrapper.
  • You cant chain it from a DOMWrapper
  • setProps does not work on newly wrapped instances, from FindComponent. (which is fine)

Reasoning behind this

Many codebases use Components, Ref or Name to search for Vue instances, when doing tricky assertions, or mounting shallow.

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 14, 2020

woah, awesome job @dobromir-hristov !

I will play with this a bit tonight and see if I can find any bugs. Looks solid, very impressive that you managed to port the entire find api. having a separate findComponent method definitely feels like a big improvement.

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

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

This is great work @dobromir-hristov

  • I don't fully understand how all of find works. That's fine - it seems like it works and the test appear to cover all the edge cases. Excellent work!
  • Can we do find(Foo).$emit with this implementation?
  • using the same resolveComponent algo from Vue core is very smart.
  • As commented I am not a fan of mutating this with __emitted. Mutation and side effects are one of the reasons VTU beta was so buggy. Unless there is a strong eason for this refactor (eg, the original implementation doesn't cover an edge case) I'm not really in favor. If there is a reason, please explain - happy to change my mind with the right argument :)
  • you have some merge conflicts.

All in all I am very excited to merge this feature up and I think a lot of people will be excited for it.

src/emitMixin.ts Show resolved Hide resolved
src/mount.ts Outdated Show resolved Hide resolved
src/utils/find.ts Show resolved Hide resolved
src/utils/matchName.ts Show resolved Hide resolved
@dobromir-hristov
Copy link
Contributor Author

  1. What part of find do you not get? Let me try to explain :)
  2. We can do find(Comp).vm.$emit.

@lmiller1990
Copy link
Member

I think I need to use a debugger a bit and play around. Will ask if I still can't understand it.

If we can revert that one mixin (top level) happy to merge this up.

Awesome we can do vm.$emit should be very few breaking changes! Your codebase should upgrade easily.

@lmiller1990 lmiller1990 self-requested a review April 17, 2020 13:21
Copy link
Member

@lmiller1990 lmiller1990 left a comment

Choose a reason for hiding this comment

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

great!

@lmiller1990
Copy link
Member

lmiller1990 commented Apr 17, 2020

Let's resolve this conflict and merge this up, I think a lot of people are waiting for this feature :D

@lmiller1990 lmiller1990 merged commit d0fd353 into master Apr 18, 2020
@lmiller1990 lmiller1990 deleted the feat/find-by-component branch April 18, 2020 14: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