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.

Sign up for GitHub

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

Jump to bottom

WIP: findComponent edge cases #184

Closed
wants to merge 2 commits into from
Closed

WIP: findComponent edge cases #184

wants to merge 2 commits into from
+358 −93

Conversation

lmiller1990
Copy link
Member

@lmiller1990 lmiller1990 commented Aug 23, 2020 •
edited
Loading

Hi all! This is a bug write-up and I would like some feedback from everyone.

TL;DR is findComponent seems very difficult support. I could be wrong but either way here is my findings and some options.

The current implementation very buggy (to the point it isn't really useful). See this issue for some context: #180

I rewrote the findVNodes function to covert some of the edge cases. How it works: basically all components have the concept of a subTree, which stores the components VDOM state. We recurse down each level via subTree or children. It works for some cases, but not for others. Here are the cases I cannot find a solution for:

  • functional components. No instance, so you cannot findComponent them, or at least when you do find them you cannot get the instance, which means no VueWrapper. I found a fix for this in this PR. But, since there is no instance, you cannot access .vm and all the stuff you would expect (like props etc). We could return a DOMWrapper, but this feels wrong. Users will expect findComponent to find a component. The whole point of splitting find and findComponent` was to emphasis the difference in the two.
  • slots and inline templates. These is the real blocker here! You can see how I access slot content in this this PR. Basically, when using slots and components with inline templates, the subTree is different and there seems not way to get the instance via recursing down.

Some (not all) these issues seem to go away when we use SFC (via vue-jest). I guess we need to dig deeper into how Vue compiles things. The original issue in #180 is fixed by using SFCs. 🤷‍♂️ Interesting. Other are not.

Here are my proposed solutions:

  • add an additional wrapper. This would something like FunctionalWrapper that returns the non-instance with some limited methods we can support (eg, html() and attributes for static HTML attributes). Not ideal.
  • dive deeper and use @vue/compiler-sfc to compile all non-SFC components to SFCs. This seems it might work. This is how vue-jest works and pre-compiled SFCs seem not to have the same caveats. This would be a long term goal, at least if I need to write the code (I also need to focus on other bugs, vue-jest, VTU v1...).
  • (my favorite solution) until we figure out how to solve the problems for non SFC components, put a warning that says "findComponent currently only supports SFCs. Non SFC support is experimental. We are working on supporting all manner of components". The reason I like this is so I don't have to keep telling people about work-arounds and triaging issues about the same thing. I'd much prefer a library with limitations that works really well, than one without limitations that doesn't. It feels more polished.

Tagging all the people who have expressed interested in contributing to and improving VTU in the last few weeks. Please give me your input!

Here is a screenshot explaining the problem:

image

image

@afontcu @dobromir-hristov @JeremyWuuuuu @JessicaSachs @cexbrayat @Jokcy @angelogulina @AtofStryker

@JessicaSachs
Copy link
Contributor

JessicaSachs commented Aug 23, 2020 via email

@afontcu
Copy link
Member

afontcu commented Aug 23, 2020

Hi!

Thanks for the write-up, Lachlan. It's been both helpful and informative.

Quick question: you mentioned that "some (not all) these issues seem to go away when we use SFC". Are we able to track down which issues are not gone when using SFC? If that's the case, I'd +1 the idea of displaying a warning.

I'm not a strong proponent or user of findComponent (as I want my tests to stay closer to the user – to the DOM), so my input here might be biased. Still, I'd really discourage implementing a FunctionalWrapper, as even though it makes sense from a code point of view, people would need to be aware of the kind of Wrapper VTU is returning, and this smells too much like an implementation detail to me.

Again, thanks!

@lmiller1990
Copy link
Member Author

@afontcu @JessicaSachs Updated above with some screenshots that may give more context!

@lmiller1990
Copy link
Member Author

lmiller1990 commented Aug 23, 2020 •
edited
Loading

@JessicaSachs dunno about running in a browser. Would need to investigate.

@afontcu yes, I think this feature sucks and no-one should use findComponent. But many people rely on it, in the same way Vue 3 is backwards compatible with the Options API we need to either figure this out or provide an alternative. +1 to discouraging the use of findComponent, though. This is the only feature that hasn't really been ported, everything else works pretty well!

The reason I bring this up is I feel like if I do not implement it likely won't happen, at least before Vue 3 is out and people would like to start upgrading. So rather than feeling bad and saying "sorry we don't support it yet" and letting issues pile up (see VTU v1) I'd much rather say "sorry, we don't that feature because it can't be done properly/impractical".

I may well be too far down the rabbit hole; if anyone is reading this that knows what I am missing here please let me know!!!!!

@jw-foss
Copy link
Contributor

jw-foss commented Aug 23, 2020

Hi, after I just read about the text and before dive into the code, I'd like to say this:
Functional component is supposed to be stateless in Vue right? Unlike what React v16+ did for functional components which have states hooks in runtime, and because of that I think accessing instance is essentially false action while there is nothing you can retrieve from the instance.
And for dynamic slots there is a field in vm.subTree named dynamicChildren might be handy for you

@lmiller1990
Copy link
Member Author

@JeremyWuuuuu hey, thanks for the feedback! I cannot find subTree for slots, lmk if you have any luck. dynamicChildren seems to only be used internally for some optimizations, it's null in all cases I've seen so far. I agree functional components should not be returned from findComponent, we can (kind of) detect them and print a warning.

I think the slots edge case will prove more challenging (also much more common). I'll continue to explore, I just wanted to give some visibility to what's going on (since we had a few issues, I don't want users to think we are just ignored/not taking the migration to Vue 3 seriously!)

Thanks for the post again, I'll see what else I can come up with! Feel free to have a hack if you like too :D

@afontcu afontcu mentioned this pull request Aug 23, 2020
Closed
@Jokcy
Copy link
Contributor

Jokcy commented Aug 24, 2020

By now, I am trying to undestand the main logic of VTU, so I copied the findAllVnodes function to my demo code and debug it. I find in the ComponentA, ComponentB demo, if I add some logic to findAllVnodes I can find all vnode and proxy I needed:

 aggregateChildren(nodes, node.children)
    if (node.component) {
      // match children of the wrapping component
      aggregateChildren(nodes, node.component.subTree.children)
      aggregateChildren(nodes, [node.component.subTree])
      if (node.component.subTree.component) {
        aggregateChildren(
          nodes,
          node.component.subTree.component.subTree.children,
        )
      }
    }

the main difference is I tried node.component.subTree.component.subTree.children and node.component.subTree, now I can find App, Main, Comp1 and Comp2.
I don't know whether it will help, I still trying hard to understand the code.

@lmiller1990
Copy link
Member Author

Interesting - you can't always assume component will exist, or subTree or children, but still good progress.

I think a recursive solution (like my PR) will make this much easier to understand. Either way - my next question is what do you get when you find Comp2? (I think ComponentB in your example? Are you using template, render or setup? It seems to make a difference).

If you can access the vnode, that is a good start. Did you try with an example using template and slots?

@Jokcy
Copy link
Contributor

Jokcy commented Aug 24, 2020 •
edited
Loading

@lmiller1990 I try to explain it.
what we need to know is the difference between subTree and children

<Comp><div>name</div></Comp>

<div>name</div> is children of Comp

{
  name: 'Comp',
  render() {
    return <div><slot /></div>
  }
}

<div><slot /></div> is subTree of Comp
So in the this demo:

const Comp1 = defineComponent({
  name: 'Comp1',
  setup() {
    return () => <div>Comp1</div>
  },
})

const Comp2 = defineComponent({
  name: 'Comp2',
  setup() {
    return () => <div>Comp2</div>
  },
})

const Main = defineComponent({
  name: 'Main',
  setup() {
    return () => (
      <div>
        <Comp1 />
        <Comp2 />
      </div>
    )
  },
})

const App = defineComponent({
  name: 'App',
  setup() {
    return () => <Main />
  },
})

if we render App, Main is subTree of App, we won't find Main in any children of any vnode. And also Main has it's own subTree, they will be in the subTree of Main, so if we ignore subTree from vnode of App(which is vnode of Main), we won't find and Component here.

I tried my demo with setup and jsx, I will try render later.

@Jokcy
Copy link
Contributor

Jokcy commented Aug 24, 2020

@lmiller1990 I try to explain it.
what we need to know is the difference between subTree and children

<Comp><div>name</div></Comp>

<div>name</div> is children of Comp

{
  name: 'Comp',
  render() {
    return <div><slot /></div>
  }
}

<div><slot /></div> is subTree of Comp
So in the this demo:

const Comp1 = defineComponent({
  name: 'Comp1',
  setup() {
    return () => <div>Comp1</div>
  },
})

const Comp2 = defineComponent({
  name: 'Comp2',
  setup() {
    return () => <div>Comp2</div>
  },
})

const Main = defineComponent({
  name: 'Main',
  setup() {
    return () => (
      <div>
        <Comp1 />
        <Comp2 />
      </div>
    )
  },
})

const App = defineComponent({
  name: 'App',
  setup() {
    return () => <Main />
  },
})

if we render App, Main is subTree of App, we won't find Main in any children of any vnode. And also Main has it's own subTree, they will be in the subTree of Main, so if we ignore subTree from vnode of App(which is vnode of Main), we won't find and Component here.

I tried my demo with setup and jsx, I will try render later.

forget it, it's not that simple...

Jokcy added a commit to Jokcy/vue-test-utils-next that referenced this pull request Aug 24, 2020
@Jokcy
fix vuejs#184
d9d2efe
@Jokcy Jokcy mentioned this pull request Aug 24, 2020
Merged
angelogulina
angelogulina reviewed Aug 24, 2020
View reviewed changes
tests/findComponent.spec.ts
}

const wrapper = mount(Parent)
wrapper.getComponent(Child)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am confused by getComponent used in this context (and some following lines as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I will abandon this PR for now since @Jokcy found a better solution.

I was just doing this as a quick test - the idea is that if the component is not found, it will throw an error (findComponent would not).

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Just for my understanding, #188 is solving the slots case but not the functional components one?

If this is true, should we pursue your proposal of having some warning or anything else?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say so. This would be a much better developer experience.

We could use this __isFunctionalComponent property I am proposing we add here #185

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it!

@lmiller1990
Copy link
Member Author

lmiller1990 commented Aug 25, 2020 •
edited
Loading

It seems someone smarter than me has figured this out #188

I will close this for now. I am sure we are likely still missing some edge cases. Anyway, this was very frustrating, but I learned a lot about Vue and I am glad we are finding all these bugs before Vue 3 is official released. Great job everyone who was involved in this discussion and has contributed so far!

@cexbrayat cexbrayat deleted the wip/find-component branch April 6, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@angelogulina angelogulina angelogulina left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lmiller1990 @JessicaSachs @afontcu @jw-foss @Jokcy @angelogulina