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

`this.$scopedSlots.default()` returns VNode directly or array of VNodes depending on content #8056

Closed
adamwathan opened this issue Apr 19, 2018 · 11 comments

Comments

@adamwathan
Copy link

@adamwathan adamwathan commented Apr 19, 2018

Version

2.5.16

Reproduction link

https://jsfiddle.net/50wL7mdz/323954/

Steps to reproduce

  1. Create a component that uses a render function to return some parent element with this.$scopedSlots.default({}) as the children.
  2. Create an instance of that component that passes multiple elements to the scoped slot and see this.$scopedSlots.default({}) return an array.
  3. Create an instance of that component that passes a single element to the scoped slot and see this.$scopedSlots.default({}) return a VNode, not an array with a single VNode in it.

What is expected?

this.$scopedSlots.default({}) should always return an array of VNodes, even if there's only one VNode in the array.

This is how this.$slots.default behaves.

What is actually happening?

this.$scopedSlots.default({}) returns mixed types: an array when there are multiple elements in the slot, or a direct VNode instance if there is only a single child.


This is inconsistent with how regular slots behave in render functions, and means any render function component rendering scoped slots as children needs to type check the result of invoking the slot to decide if it needs to be wrapped in an array:

render(h) {
  const children = this.$scopedSlots.default({})
  return h('div', {}, Array.isArray(children) ? children : [children])
}

Contrast that with regular slots where it is always safe to pass the slot as a child because it is always an array:

render(h) {
  return h('div', {}, this.$slots.default)
}

It's a bummer because although this is pretty easy to classify as a bug, it would be a breaking change for a lot of people using scoped slots to write components that use the default scoped slot as their root element:

render() {
  return this.$scopedSlots.default({ someDataThisComponentIsResponsibleFor })
}

If this bug were fixed, anyone with a component like that would need to re-write it like this:

render() {
  return this.$scopedSlots.default({ someDataThisComponentIsResponsibleFor })[0]
}

If this isn't a bug and is by design, I'd love to better understand the reasoning!

@yyx990803

This comment has been minimized.

Copy link
Member

@yyx990803 yyx990803 commented Apr 20, 2018

I was just thinking about this today. I think it makes more sense to make it consistent and always return an Array.

@adamwathan

This comment has been minimized.

Copy link
Author

@adamwathan adamwathan commented Apr 21, 2018

Cool! It would be great if it were possible to do this without breaking existing code, but the only way I can think of to do that is to allow render to return either a single node, or an array that contains a single node (so return this.$scopedSlots.default({}) would still work.)

Is that feasible, or does it sound like a horrible idea?

Something as simple as this in core/instance/render.js seems to work in my testing:

    } else {
      vnode = vm._vnode
    }
  }
  
+ if (Array.isArray(vnode) && vnode.length === 1) {
+   vnode = vnode[0]
+ }
  
  // return empty vnode in case the render function errored out
  if (!(vnode instanceof VNode)) {
    if (process.env.NODE_ENV !== 'production' && Array.isArray(vnode)) {
      warn(
        'Multiple root nodes returned from render function. Render function ' +
        'should return a single root node.',
@mekhami

This comment has been minimized.

Copy link

@mekhami mekhami commented Sep 3, 2018

Any commentary on this? I still run into this problem every day.

edit: it seems this problem was actually 'fixed' but some other libraries haven't updated to return the [0]. This problem is really confusing, though.

@Justineo

This comment has been minimized.

Copy link
Member

@Justineo Justineo commented Dec 10, 2018

@yyx990803 Are we expecting breaking changes in 2.6 if we are gonna fix this?

@yyx990803

This comment has been minimized.

Copy link
Member

@yyx990803 yyx990803 commented Dec 10, 2018

@Justineo making it all Arrays could technically be a breaking change - although could be somewhat justified as a fix. Also, a component that does not check for the value possibly being an Array will break depending on how it's being used, so I assume a decent percentage of existing code should be checking for it already.

@DominusVilicus

This comment has been minimized.

Copy link
Contributor

@DominusVilicus DominusVilicus commented Dec 22, 2018

this should pass in 2.6, although it could be considered a breaking change, its how it should work

@yyx990803 yyx990803 moved this from Todo to In progress in 2.6 Dec 26, 2018
yyx990803 added a commit that referenced this issue Dec 26, 2018
Also allow render functions to return an Array of a single element.
Close #8056
@yyx990803

This comment has been minimized.

Copy link
Member

@yyx990803 yyx990803 commented Dec 26, 2018

Implemented in c7c13c2.

Also made it possible to return an Array of a single VNode from render functions - thus avoiding breaking the usage of directly returning a scoped slot.

@yyx990803 yyx990803 closed this Dec 26, 2018
2.6 automation moved this from In progress to Done Dec 26, 2018
f2009 pushed a commit to f2009/vue that referenced this issue Jan 25, 2019
Also allow render functions to return an Array of a single element.
Close vuejs#8056
@decademoon

This comment has been minimized.

Copy link
Contributor

@decademoon decademoon commented Feb 8, 2019

return Array.isArray(res) ? res : res ? [res] : res

Technically this allows scoped slots to return falsy values, so the result won't always be an array. Is this allowed? Shouldn't it return an empty array instead?

@yyx990803

This comment has been minimized.

Copy link
Member

@yyx990803 yyx990803 commented Feb 8, 2019

@decademoon

This comment has been minimized.

Copy link
Contributor

@decademoon decademoon commented Feb 8, 2019

@yyx990803 Ah, so the current behavior is the result will always be a non-empty array of vnodes or undefined.

@yyx990803

This comment has been minimized.

Copy link
Member

@yyx990803 yyx990803 commented Feb 8, 2019

@decademoon correct!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
2.6
  
Done
6 participants
You can’t perform that action at this time.