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

fix #1474: support array class binding and events binding in stubbed functional components #1744

Merged
merged 1 commit into from Dec 7, 2020
Merged

fix #1474: support array class binding and events binding in stubbed functional components #1744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 9 additions & 36 deletions packages/create-instance/create-component-stubs.js
View file
Open in desktop
Expand Up @@ -61,28 +61,6 @@ function getCoreProperties(componentOptions: Component): Object {
}
}

function createClassString(staticClass, dynamicClass) {
// :class="someComputedObject" can return a string, object or undefined
// if it is a string, we don't need to do anything special.
let evaluatedDynamicClass = dynamicClass

// if it is an object, eg { 'foo': true }, we need to evaluate it.
// see https://github.com/vuejs/vue-test-utils/issues/1474 for more context.
if (typeof dynamicClass === 'object') {
evaluatedDynamicClass = Object.keys(dynamicClass).reduce((acc, key) => {
if (dynamicClass[key]) {
return acc + ' ' + key
}
return acc
}, '')
}

if (staticClass && evaluatedDynamicClass) {
return staticClass + ' ' + evaluatedDynamicClass
}
return staticClass || evaluatedDynamicClass
}

function resolveOptions(component, _Vue) {
if (isDynamicComponent(component)) {
return {}
Expand Down Expand Up @@ -134,24 +112,19 @@ export function createStubFromComponent(
render(h, context) {
return h(
tagName,
{
ref: componentOptions.functional ? context.data.ref : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

are all these included in context.data.attrs? Curious why so much code here can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll try to explain what I’ve done —
First, I passed full data as a second argument in createElement for functional component

{
  // Same API as `v-bind:class`, accepting either
  // a string, object, or array of strings and objects.
  class: {
    foo: true,
    bar: false
  },
  // Same API as `v-bind:style`, accepting either
  // a string, object, or array of objects.
  style: {
    color: 'red',
    fontSize: '14px'
  },
  // Normal HTML attributes
  attrs: {
    id: 'foo'
  },
  // Component props
  props: {
    myProp: 'bar'
  },
  // DOM properties
  domProps: {
    innerHTML: 'baz'
  },
  // Event handlers are nested under `on`, though
  // modifiers such as in `v-on:keyup.enter` are not
  // supported. You'll have to manually check the
  // keyCode in the handler instead.
  on: {
    click: this.clickHandler
  },
  // For components only. Allows you to listen to
  // native events, rather than events emitted from
  // the component using `vm.$emit`.
  nativeOn: {
    click: this.nativeClickHandler
  },
  // Custom directives. Note that the `binding`'s
  // `oldValue` cannot be set, as Vue keeps track
  // of it for you.
  directives: [
    {
      name: 'my-custom-directive',
      value: '2',
      expression: '1 + 1',
      arg: 'foo',
      modifiers: {
        bar: true
      }
    }
  ],
  // Scoped slots in the form of
  // { name: props => VNode | Array<VNode> }
  scopedSlots: {
    default: props => createElement('span', props.text)
  },
  // The name of the slot, if this component is the
  // child of another component
  slot: 'name-of-slot',
  // Other special top-level properties
  key: 'myKey',
  ref: 'myRef',
  // If you are applying the same ref name to multiple
  // elements in the render function. This will make `$refs.myRef` become an
  // array
  refInFor: true
}

because some property had been missed — props, on (so events don't work), nativeOn, directives, scopedSlots, slot, key, ref, refInFor. We can get all property from context.data.
Data-Object docs

class, staticClass and style property also forwarded in Data-Object. As we don't mix any class or styles, we don't need to do any preparation, because Vue processes them independently, one of accepted type — array, object or string.

In fact, we create HOC, which proxies all data:

return createElement(
      SomeComponent,
      context.data,
      context.children
    )

example from here

Also, I assign context.props and context.data.attrs in to Data-Object attrs, because Vue not rendered entries from context.props in html, if key of entries was extracted.

image

image

It standard behavior for Vue, but fix it do break current behavior vue-test-utils.

Copy link
Member

Choose a reason for hiding this comment

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

This is my main concern - since we are in 1.0 now, this is a breaking change for a lot of users. But it does make sense we have feature parity with Vue.

Maybe @afontcu has some opinion? My guess is this will only impact snapshots users - so if we make it clear they need to update their snapshots, maybe it's okay. 🤔

At this point I am not sure about the trade-off between correctness and stability.

This would need to be a minor version bump at the very least.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the better way is to request for comments of other developers and not to push breaking changes now.

Therefore, I kept the standard behavior VTU and only add support array class binding and events binding into stubbed functional components.

Copy link
Member

Choose a reason for hiding this comment

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

Right I think I misunderstood - just to clarify, this change only impacts stubbed functional components (and no change to non stubbed components, and regular stubbed components?)

If that is the case this probably isn't as big a change as I thought, and we could just make this a minor version release. Can you please clarify?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this change only impacts stubbed functional components.

I was planning this as a MINOR update, maybe even as a PATCH.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. I will make this a minor release.

domProps: componentOptions.functional
? context.data.domProps
: undefined,
attrs: componentOptions.functional
? {
componentOptions.functional
? {
...context.data,
attrs: {
...context.props,
...context.data.attrs,
class: createClassString(
context.data.staticClass,
context.data.class
)
...context.data.attrs
}
: {
}
: {
attrs: {
...this.$props
}
},
},
context
? context.children
: this.$options._renderChildren ||
Expand Down
9 changes: 8 additions & 1 deletion test/resources/components/component-with-functional-child.vue
View file
Open in desktop
@@ -1,7 +1,8 @@
<template>
<functional-component
:class="{ bar: a + b === 2, foo: a === 1, qux: a === 2 }"
:class="['baz', { bar: a + b === 2, foo: a === 1, qux: a === 2 }]"
v-text="something"
@click="handleFunctionalComponentClick"
/>
</template>

Expand All @@ -19,6 +20,12 @@ export default {
b: 1,
something: 'value'
}
},

methods: {
handleFunctionalComponentClick() {
this.something = 'newValue'
}
}
}
</script>
9 changes: 9 additions & 0 deletions test/specs/shallow-mount.spec.js
View file
Open in desktop
Expand Up @@ -33,6 +33,7 @@ describeRunIf(process.env.TEST_ENV !== 'node', 'shallowMount', () => {
it('renders dynamic class of functional child', () => {
const wrapper = shallowMount(ComponentWithFunctionalChild)
expect(wrapper.find('functional-component-stub').classes()).toContain(
'baz',
'foo',
'bar'
)
Expand All @@ -46,6 +47,14 @@ describeRunIf(process.env.TEST_ENV !== 'node', 'shallowMount', () => {
expect(wrapper.find('functional-component-stub').text()).toBe('value')
})

it('trigger click must change content of functional child', async () => {
const wrapper = shallowMount(ComponentWithFunctionalChild)

await wrapper.trigger('click')

expect(wrapper.find('functional-component-stub').text()).toBe('newValue')
})

it('returns new VueWrapper of Vue localVue if no options are passed', () => {
const compiled = compileToFunctions('<div><input /></div>')
const wrapper = shallowMount(compiled)
Expand Down