-
Notifications
You must be signed in to change notification settings - Fork 668
fix #1474: support array class binding and events binding in stubbed functional components #1744
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
fix #1474: support array class binding and events binding in stubbed functional components #1744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one question
@@ -134,24 +112,19 @@ export function createStubFromComponent( | |||
render(h, context) { | |||
return h( | |||
tagName, | |||
{ | |||
ref: componentOptions.functional ? context.data.ref : undefined, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
)
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.
It standard behavior for Vue, but fix it do break current behavior vue-test-utils.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
dev
branch.fix #xxx[,#xxx]
, where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information:
This PR fix array class binding in stubbed functional components, because previous PR add support string and object only.
Also fix events binding in stubbed functional components.
I think, best way provide all context data as the 2nd argument of h, like describe in Vue docs https://vuejs.org/v2/guide/render-function.html#Functional-Components