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

Shallow mount drops attributes on stubbed components #1377

Closed
Stoom opened this issue Dec 17, 2019 · 19 comments
Closed

Shallow mount drops attributes on stubbed components #1377

Stoom opened this issue Dec 17, 2019 · 19 comments
Assignees
Labels

Comments

@Stoom
Copy link
Contributor

Stoom commented Dec 17, 2019

Version

1.0.0-beta.30

Reproduction link

https://github.com/Stoom/vuetify-test-utils-beta30-example

Steps to reproduce

  • Create new project
  • Add vuetify
  • Use a vuetify component like v-tooltip
  • Stub out to get scoped slot
  • Assert an attribute is on the stubbed element

What is expected?

It to copy over the current attributes

What is actually happening?

custom prop attributes are lost


This works on beta28 but broke in beta29 and beta30

@JessicaSachs
Copy link
Collaborator

@Stoom Great bug report. Thank you for the detailed writeup and extra info about what version broke it.

@JessicaSachs JessicaSachs self-assigned this Dec 17, 2019
@Stoom
Copy link
Contributor Author

Stoom commented Dec 18, 2019

Any thoughts on where this might be happening?

@JessicaSachs
Copy link
Collaborator

JessicaSachs commented Dec 19, 2019

Spent a large portion of the night trying to debug. I think innerHTML of the element shows there is no issue in the actual render... so I think the issue is in the wrapper.attributes method.

I'm gonna be offline for 2 days, but here is what I've gotten to... (using VTU as the sandbox)

// stubs.spec.js
  it.only('replaces component with a component and inherits attributes', () => {
    const mounted = sandbox.stub()
    const Stub = {
      template: '<div />',
      mounted
    }
    const wrapper = mountingMethod(ComponentWithNestedChildrenAndAttributes, {
      stubs: {
        ChildComponent: Stub
      }
    })
    const childStub = wrapper.find(Stub)
    console.error('wrapper innerhtml', wrapper.vm.$el.innerHTML) // contains 'bottom' and other attrs
    console.error('attributes on childStub', childStub.attributes('bottom')) // undefined
  })

and then ComponentWithNestedChildrenAndAttributes inside of test/resources/components

<template>
  <div>
    <span>
      <child-component my-html-attribute bottom />
      <component-with-lifecycle-hooks another-html-attribute />
    </span>
  </div>
</template>

<script>
import ComponentWithChild from './component-with-child.vue'
import ComponentWithLifecycleHooks from './component-with-lifecycle-hooks.vue'

export default {
  name: 'component-with-nested-children',
  components: {
    ChildComponent: ComponentWithChild,
    ComponentWithLifecycleHooks
  }
}
</script>

@StummeJ
Copy link

StummeJ commented Feb 25, 2020

@JessicaSachs did you have any other information on this? I'm happy to try to help find the issue 😺

@JessicaSachs
Copy link
Collaborator

Heyo @StummeJ! Thanks for the ping. If you'd like to try picking this up, feel free.

I've been busy with VueConf and catching up w/ my job. The latest I have is the failing test that I wrote above.

Stoom pushed a commit to Incognitus-Io/vue-test-utils that referenced this issue Mar 9, 2020
Stoom pushed a commit to Incognitus-Io/vue-test-utils that referenced this issue Mar 9, 2020
@Stoom
Copy link
Contributor Author

Stoom commented Mar 9, 2020

Okay, so I looked back at my example and the test code you've written. It looks like with the latest dev everything works as expected. Where I see the issue is when I feed a string into the stubs object

    const wrapper = mountingMethod(ComponentWithNestedChildrenAndAttributes, {
      stubs: {
        SlotComponent: Stub,
        ChildComponent: '<div id="child-component"/>'
      }
    })

When doing this it will not error and renders without props included in the attributes. Simply replacing the string with { template: '<div id="child-component"/>' } then the test passes as expected.

@JessicaSachs my question now is, should the stubs property be able to take in a string, and if so is there something it's doing differently with a pure string over an object with the string in the template property?

@lmiller1990
Copy link
Member

lmiller1990 commented Mar 12, 2020

I think that when you pass a string like that, Vue thinks it is literally a string, like "Hello" or "Foo".

What might be happening here is it is not actually rendering a DOM element, but a string that looks like a DOM - it is escaping the < and rendering a literal <. Eg, it compiles to h('<div id....') as opposed to h('div', { id: ... }). It then inserts it via innerText, possibly.

So stubs can take a string, I think, but it is just that - a string, not a HTML template - so this is intended and correct behavior, I guess?

@StummeJ
Copy link

StummeJ commented Mar 12, 2020

@lmiller1990 it ends up going through the vue template compiler, so things like slots end up getting rendered correctly. The output of the render functions match between a string and a {template: string} stub. If you do Component: true then it gets a much more complex render function that applies $attrs and $props to the html attributes.

@lmiller1990
Copy link
Member

Sure, but if you just pass a string, I think it goes through the compiler anyway, becoming h('whatever text'). There is no second argument, which is where attrs and props would usually go.

@Stoom
Copy link
Contributor Author

Stoom commented Mar 13, 2020

Sorry for the multiple accounts, @StummeJ is for my professional job... That kind of makes sense, but it seems like there are different behaviors given similar render functions...


SlotComponent: { template: '<div id="SlotComponent"/>' }
function anonymous() {
  with(this){return _c('div',{attrs:{"id":"SlotComponent"}})}
}

ChildComponent: '<div id="child-component"/>'
function anonymous() {
  with(this){return _c('div',{attrs:{"id":"child-component"}})}
}

OriginalComponent: true
function render(h, context) {
      var this$1 = this;

      return h(tagName, {
        ref: componentOptions.functional ? context.data.ref : undefined,
        attrs: componentOptions.functional ? Object.assign({}, context.props, context.data.attrs, { class: createClassString(context.data.staticClass, context.data.class) }) : Object.assign({}, this.$props)
      }, context ? context.children : this.$options._renderChildren || getScopedSlotRenderFunctions(this).map(function (x) {
        return this$1.$options.parent._vnode.data.scopedSlots[x]();
      }));
    }

output:

<div>
  <span>
    <div id="SlotComponent" prop1="foobar" prop2="fizzbuzz"></div>
    <div id="child-component"></div>
    <originalcomponent-stub prop2="fizzbuzz" prop1="foobar"></originalcomponent-stub> 
 </span>
</div>

@lmiller1990
Copy link
Member

Oh, I did not realize the second one rendered like that. Interesting.

Rather than making the codebase more complex to support passing a <div id="blah />, I think it's probably find to just update the docs and remove the example saying you can pass a string that is valid HTML. It seems like passing a { template } is working as expected, as you noted. What do you think? I'm a fan of keeping it simple, rather than providing 3-4 different ways to accomplish the same thing.

@Stoom
Copy link
Contributor Author

Stoom commented Mar 14, 2020

Should we also update the codebase so that when a string is passed, it's wrapped in the { template } object and consol warn saying it's deprecated as well? That way in future versions we could also change the typing:

type Stubs = {
  [key: string]: Component | string | boolean
} | string[]

@lmiller1990
Copy link
Member

There is a valid use case though, if you want your stub to be a string literal: eg something like 'Hello' would compile to h('Hello'), which is valid. We could add warning checking for HTML markup, < and >, with a message saying something like "passing HTML as a string is not supported, please pass a component with a render function or a template field. What do you think?

@Stoom
Copy link
Contributor Author

Stoom commented Mar 15, 2020

I don't think that would work with the string variant. It looks like it tries to compile the string as a vue template, so not passing a valid template will error out. Really it looks like the string form and the template form are 99% the same, except for props, and looking more at it, it disables stubbing child components.

Here's what I was thinking and a little more about the reason behind it. Like I said above, just translate a string form into the template form. It looks like the string version follows a similar path, just simpler / maybe uncomplete. It's hard to tell intent behind the string form.

Either way, if you try to do some like '<my-custom-component-stub />' it will error out in either of these methods. Kinda a bummer because I'd love to have the same effect as the boolean method, but I understand now how to target a stub in the wrapper.find better.

Here are the snippets I think are relative:


create-component-stub.js:L148

function createStubFromString(
  templateString: string,
  originalComponent: Component = {},
  name: string,
  _Vue: Component
): Component {
  if (templateContainsComponent(templateString, name)) {
    throwError('options.stub cannot contain a circular reference')
  }
  const componentOptions = resolveOptions(originalComponent, _Vue)

  return {
    ...getCoreProperties(componentOptions),
    $_doNotStubChildren: true,
    ...compileFromString(templateString)
  }
}

create-component-stub.js:L172

export function createStubsFromStubsObject(
  originalComponents: Object = {},
  stubs: Object,
  _Vue: Component
): Components {
  return Object.keys(stubs || {}).reduce((acc, stubName) => {
    const stub = stubs[stubName]

    validateStub(stub)

    if (stub === false) {
      return acc
    }

    if (stub === true) {
      const component = resolveComponent(originalComponents, stubName)
      acc[stubName] = createStubFromComponent(component, stubName, _Vue)
      return acc
    }

    if (typeof stub === 'string') {
      const component = resolveComponent(originalComponents, stubName)
      acc[stubName] = createStubFromString(stub, component, stubName, _Vue)
      return acc
    }

    if (componentNeedsCompiling(stub)) {
      compileTemplate(stub)
    }

    acc[stubName] = stub
    stub._Ctor = {}

    return acc
  }, {})
}

compile-template.js:L7

export function compileFromString(str: string) {
  if (!compileToFunctions) {
    throwError(
      `vueTemplateCompiler is undefined, you must pass ` +
        `precompiled components if vue-template-compiler is ` +
        `undefined`
    )
  }
  return compileToFunctions(str)
}

export function compileTemplate(component: Component): void {
  if (component.template) {
    if (component.template.charAt('#') === '#') {
      var el = document.querySelector(component.template)
      if (!el) {
        throwError('Cannot find element' + component.template)

        el = document.createElement('div')
      }
      component.template = el.innerHTML
    }

    Object.assign(component, compileToFunctions(component.template))
  }

  if (component.components) {
    Object.keys(component.components).forEach(c => {
      const cmp = component.components[c]
      if (!cmp.render) {
        compileTemplate(cmp)
      }
    })
  }

  if (component.extends) {
    compileTemplate(component.extends)
  }

  if (component.extendOptions && !component.options.render) {
    compileTemplate(component.options)
  }
}

@Stoom
Copy link
Contributor Author

Stoom commented Mar 15, 2020

Either way the documentation needs to be updated since the API shows one thing, but the mounting options shows another:

https://vue-test-utils.vuejs.org/api/options.html#stubs
https://vue-test-utils.vuejs.org/api/#mount

@Stoom
Copy link
Contributor Author

Stoom commented Mar 15, 2020

Digging into the compileStubFromString and I got the string to emit props in the html. The key is when compiling the template as a string we get the options with null typed props. If I force props to undefined then the component will render the props correctly

function createStubFromString(
  templateString: string,
  originalComponent: Component = {},
  name: string,
  _Vue: Component
): Component {
  if (templateContainsComponent(templateString, name)) {
    throwError('options.stub cannot contain a circular reference')
  }
  const componentOptions = resolveOptions(originalComponent, _Vue)

+  const coreProps = getCoreProperties(componentOptions)
+  coreProps.props = undefined

  return {
-     ...getCoreProperties(componentOptions)
+    ...coreProps,
    $_doNotStubChildren: true,
     ...compileFromString(templateString)
  }
}
<div><span><div id="SlotComponent" prop1="foobar" prop2="fizzbuzz"></div> <div id="child-component" prop1="foobar" prop2="fizzbuzz"></div> <originalcomponent-stub prop2="fizzbuzz" prop1="foobar"></originalcomponent-stub></span></div>

string component:

{
  render: [Function: anonymous],
  staticRenderFns: []
}

string options:

{
  attrs: undefined,
  name: 'component-with-props',
  model: undefined,
  props: { prop2: { type: null }, prop1: { type: null } },
  on: undefined,
  key: undefined,
  domProps: undefined,
  class: undefined,
  staticClass: undefined,
  staticStyle: undefined,
  style: undefined,
  normalizedStyle: undefined,
  nativeOn: undefined,
  functional: undefined
}

template:

{
  template: '<div id="SlotComponent"/>',
  render: [Function: anonymous],
  staticRenderFns: []
}

@lmiller1990
Copy link
Member

Wow, great digging! I didn't expect this to work (with so few changes, too).

What do you think? I'm pretty happy just do disallow passing strings. Keeps it simple. Then we just update the docs, and do a console.warn if someone passes a string. Thoughts?

@Stoom
Copy link
Contributor Author

Stoom commented Mar 16, 2020

I'll make a PR with what I'm thinking and we can take it from there 😃

Stoom pushed a commit to Incognitus-Io/vue-test-utils that referenced this issue Mar 17, 2020
close vuejs#1377  This also fixes the component name being dropped for template stubs
@StummeJ
Copy link

StummeJ commented Mar 18, 2020

PR #1473 has been opened 😸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants