WIP: detect multiple root nodes #25

dobromir-hristov · 2020-03-27T14:13:07Z

Description

This PR finds the component's component instance inside the mounted parent vm.
Then adds a property to note if it has multiple root nodes.

This should be then used to aid methods like find or classes as they cannot work with multiple root nodes as of now.

Related to #19

What next?

I need to check whether we can do the same queries from the component's own element, not the mounting parent, even though they should be the same.

Fine Print
This took allot of digging, but I finally got the hang of things 😆

src/vue-wrapper.ts

dobromir-hristov · 2020-03-27T16:42:13Z

src/vue-wrapper.ts

    return Array.from(results).map((x) => new DOMWrapper(x))
  }

  async setChecked(checked: boolean = true) {
-    return new DOMWrapper(this.vm.$el).setChecked(checked)
+    return new DOMWrapper(this.element).setChecked(checked)


Here it would probably be best to return an error, if the component has multiple roots?

as discussed I think we should omit this code entirely for now and revisit it later - setChecked on a VueWrapper may not make sense after all, I am in favor of only supporting it on DOMWrapper.

OK. We can add them back later I guess.

dobromir-hristov · 2020-03-27T16:42:30Z

src/vue-wrapper.ts

  }

  trigger(eventString: string) {
-    const rootElementWrapper = new DOMWrapper(this.vm.$el)
+    const rootElementWrapper = new DOMWrapper(this.element)


Here it would probably be best to return an error, if the component has multiple roots?

dobromir-hristov · 2020-03-27T16:44:05Z

src/vue-wrapper.ts

@@ -26,15 +43,15 @@ export class VueWrapper implements WrapperAPI {
  }

  html() {
-    return this.vm.$el.outerHTML
+    return this.element.outerHTML


We can probably do innerHtml for multiple root nodes?

what does outer html return? Whatever makes most sense!

outerHtml returns the mount point, which is in this case div#app :/

sure, makes sense. I think we need outerHTML for a single node still? (not sure)

Correct. It will be a check for multiRoot

dobromir-hristov · 2020-03-27T16:44:49Z

src/vue-wrapper.ts

+        this.componentVM.$el.parentElement
+      : this.componentVM.$el
+  }
+
  classes(): string[] {
    throw Error('TODO: Implement VueWrapper#classes')


We should error out if classes is used with multiple nodes.

lmiller1990 · 2020-03-28T01:48:00Z

Some interesting stuff here. Before getting to into a code review, I have some thoughts:

the fact this complexity is introduced, and we might have throw an error if there are multiple nodes feels like a bit of a red flag. Suddenly the users needs to understand the implementation details.

I'm starting to think we should support setChecked etc on VueWrapper, only DOM Wrapper. My reasoning is that components do not get checked or change state - DOM nodes do. We will still need to detect multiple root nodes, either way, so this PR is still useful (eg, for find, we need to apply it on all root node's querySelector somehow).

Take two cases:

Case 1:

<template>
  <input id="foo" type="checkbox" />
</template>

Case 2:

<template>
  <input id="foo" type="checkbox" />
  <input id="bar" type="checkbox" />
</template>

Case 1 wrapper.setChecked works, and case 2 wrapper.setChecked will error out... not ideal.

Should we just remove setChecked (and other methods like trigger) entirely from VueWrapper? As long as we make find work on the root node(s), it will be fine, and I'd argue, a little less magic and more explicit. Then you do:

Case 1: wrapper.find('#foo').setChecked
Case 2: wrapper.find('#foo').setChecked

It's the same for both. Then the user does not need to think about how many root nodes there are, and we don't need to deal with the complexity. You just find your DOM element, and call setChecked. I think it is more intuitive to setChecked on a DOM element as well.

Thoughts? @afontcu and @dobromir-hristov ?

dobromir-hristov · 2020-03-28T09:31:22Z

As most of the times, we just invoke the DOMWrapper methods, it makes sense to remove it from the VueWrapper.
Question is, would we allow find to return a Vue Wrapper, like VTU Beta does now? If so, then its not ideal. If no, then you would rarely invoke setChecked on your own Vue mounted component.

As for find searching in it's root, we would have to use the parent element for searching.
This ofc only works for DOM query selection, and if we introduce more advanced search methods, it would fail. But its a discussion for another ticket :)

lmiller1990

Left some minor comments, mainly around private on the hasMultipleRoots

src/mount.ts

lmiller1990 · 2020-03-29T08:45:31Z

src/vue-wrapper.ts

    this.__emitted = events
  }

+  get __hasMultipleRoots(): boolean {


instead of using __ we can just make this private and it will only be accessible in the class (and you don't need __ anymore)

private get hasMultipleRoots(): boolean

alright then

lmiller1990 · 2020-03-29T08:46:35Z

src/vue-wrapper.ts

@@ -26,15 +43,15 @@ export class VueWrapper implements WrapperAPI {
  }

  html() {
-    return this.vm.$el.outerHTML
+    return this.element.outerHTML


sure, makes sense. I think we need outerHTML for a single node still? (not sure)

lmiller1990 · 2020-03-29T08:47:21Z

src/vue-wrapper.ts

    return Array.from(results).map((x) => new DOMWrapper(x))
  }

  async setChecked(checked: boolean = true) {
-    return new DOMWrapper(this.vm.$el).setChecked(checked)
+    return new DOMWrapper(this.element).setChecked(checked)


as discussed I think we should omit this code entirely for now and revisit it later - setChecked on a VueWrapper may not make sense after all, I am in favor of only supporting it on DOMWrapper.

lmiller1990 · 2020-03-29T08:47:27Z

src/vue-wrapper.ts

  }

  trigger(eventString: string) {
-    const rootElementWrapper = new DOMWrapper(this.vm.$el)
+    const rootElementWrapper = new DOMWrapper(this.element)


dobromir-hristov · 2020-03-29T09:35:23Z

We can remove the other methods in another PR.

lmiller1990 · 2020-03-29T09:59:17Z

learned a lot from this PR about internals of Vue 3, great!

dobromir-hristov requested review from afontcu and lmiller1990 March 27, 2020 14:13

afontcu reviewed Mar 27, 2020

View reviewed changes

src/vue-wrapper.ts Show resolved Hide resolved

dobromir-hristov requested a review from afontcu March 27, 2020 16:38

dobromir-hristov self-assigned this Mar 27, 2020

dobromir-hristov commented Mar 27, 2020

View reviewed changes

dobromir-hristov mentioned this pull request Mar 28, 2020

find and findAll #7

Closed

lmiller1990 reviewed Mar 29, 2020

View reviewed changes

dobromir-hristov added 5 commits March 29, 2020 12:16

feat: detect multiple root nodes

1cf1522

refactor: use ref instead of reaching in

eb2c64c

refactor: fix types

bcfb13d

feat: fallback to parent element if current element has multiple roots.

6d6e7ae

refactor: make hasMultipleRoots a private property

ccb460b

dobromir-hristov force-pushed the feat/detect-multiple-root-nodes branch from 8e05ee4 to ccb460b Compare March 29, 2020 09:34

dobromir-hristov marked this pull request as ready for review March 29, 2020 09:35

lmiller1990 merged commit 6a21b8c into master Mar 29, 2020

lmiller1990 deleted the feat/detect-multiple-root-nodes branch March 29, 2020 09:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: detect multiple root nodes #25

WIP: detect multiple root nodes #25

dobromir-hristov commented Mar 27, 2020 •

edited

Loading

dobromir-hristov Mar 27, 2020

lmiller1990 Mar 29, 2020

dobromir-hristov Mar 29, 2020

dobromir-hristov Mar 27, 2020

lmiller1990 Mar 29, 2020

dobromir-hristov Mar 27, 2020

lmiller1990 Mar 28, 2020

dobromir-hristov Mar 28, 2020

lmiller1990 Mar 29, 2020

dobromir-hristov Mar 29, 2020

dobromir-hristov Mar 27, 2020

lmiller1990 commented Mar 28, 2020 •

edited

Loading

dobromir-hristov commented Mar 28, 2020

lmiller1990 left a comment

lmiller1990 Mar 29, 2020

dobromir-hristov Mar 29, 2020

lmiller1990 Mar 29, 2020

lmiller1990 Mar 29, 2020

lmiller1990 Mar 29, 2020

dobromir-hristov commented Mar 29, 2020

lmiller1990 commented Mar 29, 2020

WIP: detect multiple root nodes #25

WIP: detect multiple root nodes #25

Conversation

dobromir-hristov commented Mar 27, 2020 • edited Loading

Description

What next?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmiller1990 commented Mar 28, 2020 • edited Loading

dobromir-hristov commented Mar 28, 2020

lmiller1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dobromir-hristov commented Mar 29, 2020

lmiller1990 commented Mar 29, 2020

dobromir-hristov commented Mar 27, 2020 •

edited

Loading

lmiller1990 commented Mar 28, 2020 •

edited

Loading