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

vnode not work for functional from v2.5.14 #7799

Closed
yozman opened this issue Mar 12, 2018 · 26 comments
Closed

vnode not work for functional from v2.5.14 #7799

yozman opened this issue Mar 12, 2018 · 26 comments

Comments

@yozman
Copy link

yozman commented Mar 12, 2018

Version

2.5.15

Reproduction link

https://codepen.io/yozman/pen/JLdaRZ

Steps to reproduce

the codepen is a demo for v2.5.13,
it's works well,
when change to v2.5.14 or v2.5.15,
there is blank output

What is expected?

work well as v2.5.13

What is actually happening?

blank output

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@yyx990803
need ur help,
could u plz fix it.

@LinusBorg
Copy link
Member

Mentioning Evan will not get your help faster, we read every issue and priorize all issues accoring to the grater needs of Vue and the ecosystem.

@jkzing
Copy link
Member

jkzing commented Mar 12, 2018

By using v-bind="item", you are converting a VNode instance into a "vnode like" object, that's the reason you are not seeing your expected result.

Although that usage worked in previous version, I recommend to use a more standard way:

Vue.component('value-vnode', {
  functional: true,
  render: (h, { props }) => props.value
})

<div>
  <op-vnode :value="$slots.default"/>
</div>

@posva
Copy link
Member

posva commented Mar 12, 2018

hhm, not sure you should v-bind the whole vnode, the type gets lost. If you only need to bind one value, just use one single prop:

pretty much what @jkzing said (as I was writing this before his comment got published 😆 )

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Mar 12, 2018

OP's usage should be covered in #7765.

For more board usage, should we support v-bind for non-plain object?

On the other hand, returning a non vnode data in render function doesn't make much sense. It might invalidate JS engine's type speculation, which might harm runtime performance(not sure).

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@jkzing
maybe there should be a helper function createVNode
to change vnode like to vnode
or how can I use this cloneVNode?

@jkzing
Copy link
Member

jkzing commented Mar 12, 2018

Actually I'm pretty agree with @HerringtonDarkholme, that render function should return only vnode data. As returning non vnode object was working in previous version, a warning would be more reasonable than a fix?

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@jkzing
I don't think so.
v-bind is a alias like jsx {...props}
but in this case it not work like {...props}

what is v-bind mean ?
bind something whatever what it is,
if u don't want to support this,
u guys should change v-bind to v-bind.object I think

@HerringtonDarkholme
Copy link
Member

bind something whatever what it is,

Sadly that's not how Vue works at all.

Vue's vnode has two domProps and attrs fields and they are handled differently internally.
This means Vue has to create two separate objects to extract props and attrs. https://github.com/vuejs/vue/blob/dev/src/core/instance/render-helpers/bind-object-props.js#L42-L44

On the other hand, React flavor's JSX only have one props. So passing object as is makes sense.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@HerringtonDarkholme
not matter what Vue work as,
the meaning is for v-bind should be,
or u guys should change v-bind to v-bind:object

@LinusBorg
Copy link
Member

Well, you don't get to define what v-bind means.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@HerringtonDarkholme @jkzing @LinusBorg
it's works well untill 2.5.13
should this be a breaking change?

@yozman
Copy link
Author

yozman commented Mar 12, 2018

if it is a breaking change,
should be on 3.x
not 2.x

@LinusBorg
Copy link
Member

LinusBorg commented Mar 12, 2018

it's works well untill 2.5.13
should this be a breaking change?

It was never intended to work, so no, it's not a breaking change

or u guys should change v-bind to v-bind:object

Now that, on the other hand, would be a very big breaking change, because props named object would no longer work as expected.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@LinusBorg
so u guys should fix it, isn't it?

@LinusBorg
Copy link
Member

LinusBorg commented Mar 12, 2018

Ringt Now I still don't see what we should fix, since what you did should not have worked in the first place, as far as I can tell so far.

So no, as far as I understand right now, I don't see anything to "fix", but maybe we find a way to improve the behaviour.

We could also document this a bit better if no improve men can be found.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@LinusBorg
the key point is at render when return vnode like it works untill 2.5.13
but now it doesn't work,
and u guys say it shouldn't work.
ok, could u please give some api like cloneVNode to make it work as before?
or where I can import VNode class?

@jkzing
Copy link
Member

jkzing commented Mar 12, 2018

@yozman, I already gave you a proper usage above, which is:
when you want to pass vnode via props, rather than v-bind the whole vnode, explicitly bind it to a prop like <foo v-bind:some-node="$vnode">.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@jkzing
I think it's a trick not a solution
it's ok if u guys don't have v-bind to do like jsx {...props}
but now it's support, so I think it should support v-bind too.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@jkzing
I have ui library support thousand of user,
it's a fxxking breaking change u know???!!!

@LinusBorg
Copy link
Member

There's no need to be swearing. I will simply lock the issue if you can't discuss in a civil manner.

@javoski
Copy link
Member

javoski commented Mar 12, 2018

That's not v-bind(without an argument) works for, and here's an example explained why it shouldn't be used it in that way.

If you really persist in using v-bind(without an argument), you can do it like this:

Vue.component('op-vnode', {
  functional: true,
  render: (h, { props }) => props.node,
});

Vue.component('op-row', {
  template: `
    <div>
      <template v-for="(item, index) in $slots.default">
        <span v-if="index">&gt;</span>
        <op-vnode v-bind="{ node: item }" />
      </template>
    </div>
  `,
});

@yyx990803
Copy link
Member

yyx990803 commented Mar 12, 2018

  1. This is not a breaking change because it was never intended to work. It is only considered a breaking change if the usage was explicitly documented in the public API.

  2. Just because your library has thousands of users doesn't mean you get to demand arbitrary changes in Vue.

  3. A valid workaround has been suggested multiple times, all you need to do is update your implementation and publish a patch release of your library, instead of waiting for Vue to change.

  4. You've received a warning for swearing. If you do that again you will be blocked from all Vue repositories.

@yozman
Copy link
Author

yozman commented Mar 12, 2018

got it, sorry for my bad words

@yozman
Copy link
Author

yozman commented Mar 12, 2018

@LinusBorg @jkzing @yyx990803
there must be some trouble on vnode,
in my other case

<op-query-form>

import OpButton from '../op-button';
import OpCol from '../op-col';
import OpForm from '../op-form';
import OpRow from '../op-row';
import OpVnode from '../op-vnode';
import slots from '../../src/mixins/slots';

const template = /*html*/`
  <op-form>
    <op-row v-for="(row, rowIndex) in grid" :key="rowIndex" :gutter="70" align="bottom" type="flex">
      <op-col v-for="(col, colIndex) in row" :key="colIndex" :span="6">
        <template v-if="col.name === 'control'">
          <op-vnode v-for="(control, index) in col" :key="index" :clone="control"/>
          <template v-if="slots().length > 7">
            <op-button type="text" @click="expanded = !expanded" :style="{ 'margin-top': '16px' }">{{ expanded ? '收起' : '展开' }}</op-button>
          </template>
        </template>
        <op-vnode v-else :clone="col"/>
      </op-col>
    </op-row>
    <b hidden><slot/></b>
  </op-form>
`;

export default {
  template,

  components: {
    OpButton,
    OpCol,
    OpForm,
    OpRow,
    OpVnode,
  },

  mixins: [slots],

  data: () => ({
    expanded: false,
  }),

  computed: {
    grid: ({ expanded, getGrid, slots: _slots }) => getGrid(
      _slots().reduce(getGrid, []),
      _slots('control'),
      expanded ? _slots().length : Math.min(_slots().length, 7) % 4,
    ),
  },

  methods: {
    getGrid(arr, item, index) {
      if (!this.expanded && index >= 7) return arr;
      if (index % 4 === 0) return [...arr, [item]];
      arr[arr.length - 1].push(item);
      return arr;
    },
  },
};

as I know $slots is not computed so I add <b hidden><slot/></b> to make it observerable and I use like this

import OPUI from '@wnpm/op-ui';

const template = /*html*/`
  <div>
    <op-breadcrumb separator="/">
      <op-breadcrumb-item>首页</op-breadcrumb-item>
      <op-breadcrumb-item>YDC</op-breadcrumb-item>
      <op-breadcrumb-item>查询表格</op-breadcrumb-item>
    </op-breadcrumb>
    <op-card>
      <h2 slot="out-header">任务分组</h2>
      <op-query-form>
        <template slot="control">
          <op-button type="primary">查询</op-button>
          <op-button>清空</op-button>
        </template>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-select v-model="select">
            <op-option value="北京市朝阳区"/>
            <op-option value="北京市海淀区"/>
            <op-option value="北京市东城区"/>
          </op-select>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-date-picker v-model="date"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-time-picker v-model="time"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
        <op-form-item label="销售地点">
          <op-input placeholder="请填写"/>
        </op-form-item>
      </op-query-form>
    </op-card>
  </div>
`;

const component = {
  template,

  components: {
    ...OPUI,
  },

  data: () => ({
    fullscreen: false,
    select: '',
    date: '',
    time: '',
  }),
};

export default {
  component,

  path: 'list',
};

when change select view didn't update,
everything is works well untill 2.5.13 use v-bind

@yyx990803
Copy link
Member

@yozman please open a separate issue with proper reproduction. This thread is not your personal support line.

@vuejs vuejs locked and limited conversation to collaborators Mar 12, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants