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

Initial proposal of breaking changes in 3.0 #2

Closed
yyx990803 opened this issue Sep 22, 2018 · 99 comments
Closed

Initial proposal of breaking changes in 3.0 #2

yyx990803 opened this issue Sep 22, 2018 · 99 comments

Comments

@yyx990803
Copy link
Member

yyx990803 commented Sep 22, 2018

Changes

Props

Component no longer need to delcare props in order to receive props. Everything passed from parent vnode's data (with the exception of internal properties, i,e. key, ref, slots and nativeOn*) will be available in this.$props and also as the first argument of the render function. This eliminates the need for this.$attrs and this.$listeners.

When no props are declared on a component, props will not be proxied on the component instance and can only be accessed via this.$props or the props argument in render functions.

You still can delcare props in order to specify default values and perform runtime type checking, and it works just like before. Declared props will also be proxied on the component instance. However, the behavior of undeclared props falling through as attrs will be removed; it's as if inheritAttr now defaults to false. The component will be responsible for merging the props as attrs onto the desired element.

VNodes

Flat Data Format

// before
{
  attrs: { id: 'foo' },
  domProps: { innerHTML: '' },
  on: { click: foo },
  key: 'foo',
  ref: 'bar'
}

// after (consistent with JSX usage)
{
  id: 'foo',
  domPropsInnerHTML: '',
  onClick: foo,
  key: 'foo',
  ref: ref => {
    this.$refs.bar = ref
  }
}
  • Less memory allocation & faster diffs
  • Makes it consistent with JSX
  • Easier spread operations

VNodes are now context-free

h can now be globally imported and is no longer bound to component instacnes. VNodes created are also no longer bound to compomnent instances (this means you can no longer access vnode.context to get the component instance that created it)

Component in Render Functions

No longer resolves component by string names; Any h call with a string is considered an element. Components must be resolved before being passed to h.

import { resolveComponent } from 'vue'

render (h) {
  // only necessary when you are trying to access a registered component instead
  // of an imported one
  const Comp = resolveComponent(this, 'foo')
  return h(Comp)
}

In templates, components should be uppercase to differentiate from normal elements.

NOTE: how to tell in browser templates? In compiler, use the following intuitions:

  1. If uppercase -> Component
  2. If known HTML elements -> element
  3. Treat as unknown component - at runtime, try resolving as a component first, if not found, render as element. (resolveComponent returns name string if component is not found)

Slots

Unifying Normnal Slots and Scoped Slots

Scoped slots and normal slots are now unified. There's no more difference between the two. Inside a component, all slots on this.$slots will be functions and all them can be passed arguments.

Usage Syntax Change

// before
h(Comp, [
  h('div', { slot: 'foo' }, 'foo')
  h('div', { slot: 'bar' }, 'bar')
])

// after
h(Comp, () => h('div', 'default slot'))

// or
import { childFlags } from 'vue/flags'

h(Comp, null, {
  slots: {
    foo: () => h('div', 'foo'),
    bar: () => h('div', 'bar')
  }
}, childFlags.COMPILED_SLOTS)

// also works
h(Comp, null, {
  foo: () => h('div', 'foo'),
  bar: () => h('div', 'bar')
})

Functional Component

Functional components can now really be just functions.

// before
const Func = {
  functional: true,
  render (h, ctx) {
    return h('div')
  }
}

// Now can also be:
const Func = (h, props, slots, ctx) => h('div')
Func.pure = true

Async Component

Async components now must be explicitly created.

import { createAsyncComponent } from 'vue'

const AsyncFoo = createAsyncComponent(() => import('./Foo.vue'))

Directives

  • Now are internally on-vnode hooks with the exact same lifecycle as components.

  • Custom directives are now applied via a helper:

import { applyDirective, resolveDirective } from 'vue'

render (h) {
  // equivalent for v-my-dir
  const myDir = resolveDirective(this, 'my-dir')
  return applyDirective(h('div', 'hello'), [[myDir, this.someValue]])
}

Styles

No longer performs auto-prefixing.

Attributes

  • No longer auto coerces boolean or enumerated attribute values.
  • No longer removes attribute if value is boolean false. Instead, it's set as attr="false" instead. To remove the attribute, use null.

Filters

Filters are gone for good (or can it?)

Refs

  • Function refs are now supported.
  • String refs are no longer supported in render functions (now only supported in templates and compiled into function refs)
  • String refs no longer automatically generates an array when used with v-for. Instead, use something like :ref="'foo' + key" or function refs.
@yyx990803
Copy link
Member Author

/cc @vuejs/collaborators @octref @atinux @DanielRosenwasser @alexchopin @clarkdo @pi0 @chenjiahan @johnleider @Leopoldthecoder @KaelWD @icarusion @rstoenescu @rigor789 @Hanks10100

You have been added to this repo either because you are a core team member or as maintainer of notable projects that builds on top of Vue. I'm giving you early access to the WIP repo to provide early feedback on proposed breaking changes to Vue internals. What I am most interested in is how much would the above changes affect your project - for each change, how difficult would it be to adapt to it? Would any of these be a deal breaker? Any of these would really help? Any additional ideas? Any type of feedback is welcome - also keep in mind that this is very early stage and nothing is set in stone yet.

@Justineo
Copy link
Member

Hi Evan,

Nice work! Here are some of my early thoughts:

Props

However, the behavior of undeclared props falling through as attrs will be removed; it's as if inheritAttr now defaults to false. The component will be responsible for merging the props as attrs onto the desired element.

Does this mean component users cannot output arbitrary attributes onto the root element unless the component authors explicitly allow this? I think that might cause some troubles because component authors usually cannot know in advance what attributes are necessary in certain use cases (mostly interoperability issues). eg. A11Y related stuff aria-*/role or Microdata's itemprop/itemtype/... or some necessary attributes when leveraging some existing frontend libraries that depend them.

VNodes

VNodes are now context-free

This seem that we cannot access components inside directives anymore (which we do quite a lot currently in our projects). I personally prefer directives over components on certain use cases and I haven't think of a way to migrate without drastically breaking our current API ATM.

Component in Render Functions

In templates, components should be uppercase to differentiate from normal elements.

Does this mean we no longer allow kebab-casing for Vue components in templates?

NOTE: how to tell in browser templates?

Just skip step 1 for compiler intuitions seems fine?

Slots

Unifying Normnal Slots and Scoped Slots

Great. Though this might hurt those components providing slots and scoped slots with the same name (but for different purposes). It already doesn't work as expected when using template, while those who are using render functions for this might gonna change slot names.

Directives

  • Custom directives are now applied via a helper:

Does this only affects render functions? Is there any difference for templates?

Attributes

No longer removes attribute if value is boolean false. Instead, it's set as attr="false" instead. To remove the attribute, use null.

Do we still have predefined boolean attributes (true boolean attrs like disabled/checked, not boolean-ish strings like draggable)?

@KaelWD
Copy link
Contributor

KaelWD commented Sep 22, 2018

might gonna change slot names

You could probably use slot.length to differentiate between scoped and not.

seem that we cannot access components inside directives anymore

I think we use this in a few places too.

@znck
Copy link
Member

znck commented Sep 22, 2018

However, the behavior of undeclared props falling through as attrs will be removed; it's as if inheritAttr now defaults to false.

What I get here is: this.$props would have all the props and there is no this.$attrs which means if I have to forward unhandled attributes to an element I have to calculate this.$attrs equivalent. Right?

@znck
Copy link
Member

znck commented Sep 22, 2018

VNodes are now context-free

I guess this would affect devtools. /cc @Akryum

@znck
Copy link
Member

znck commented Sep 22, 2018

 // after (consistent with JSX usage)
{
  type: [Function MyComponent], // type of vnode, some sort of reference to component.
  id: 'foo',
  domPropsInnerHTML: '',
  onClick: foo,
  key: 'foo',
  ref: ref => {
    this.$refs.bar = ref
  }
}

Maybe we can add type to vnode so that it's easy do the check vnode.type === MyComponent.

@Kingwl
Copy link
Member

Kingwl commented Sep 22, 2018

Flat Data Format

seems friendly to tsx, should we add event(emit) declaration into component?
similar to props declaration, you can access the declaration of the event in component instance, or $emit the dynamic event

// Now can also be:
const Func = (h, props, slots, ctx) => h('div')
Func.pure = true

why we need the pure props?

Filters are gone for good (or can it?)

a nice sugar for template

@eddyerburgh
Copy link
Member

why we need the pure props?

I think it's to identify functional components from components created with Vue.extend/ async components. But I think we could add a flag to extended components, and omit pure from functional components.

@phanan
Copy link
Member

phanan commented Sep 22, 2018

Filters are gone for good (or can it?)

a nice sugar for template

Oh man, I can still recall the chaos it created when the same proposal was raised for v2. Too bad it's been a while and pipe operator is still not a thing yet. FWIW, all the arguments (for both sides) in the linked issue should still be valid.

Edit: Yes, they can be gone now that users are more used to a v2 world with limited support for filters :)

@znck
Copy link
Member

znck commented Sep 22, 2018

What would be the prop merging behavior for class and event props (onClick)?

@HerringtonDarkholme
Copy link
Member

HerringtonDarkholme commented Sep 22, 2018

I share the same concern with @Justineo . Other than ariia/itemprop like attributes, class is a common usage of prop falling. Migration tool might help but code modification seems to be unavoidable and tedious.

why we need the pure props?

@Kingwl I guess it might be used to differentiate between class component or Vue.extend from pure functional component.
pure seems to be optimization hint.
https://github.com/vuejs/vue-next/blob/8de1c484ff2c9bab81f1a93fcb58f53859ff0227/packages/core/src/createRenderer.ts#L558-L560

@Kingwl
Copy link
Member

Kingwl commented Sep 22, 2018

@phanan oh, i forget the pipeline operator syntax 🤣
you are right, drop it
Although pipeline is still stage 1

@Leopoldthecoder
Copy link
Contributor

I also share some of the concerns @Justineo has.

component authors usually cannot know in advance what attributes are necessary in certain use cases

Without $attrs, we may have to manually filter out all explicit component APIs from props and attach what's left of it to the desired element.

we cannot access components inside directives anymore

This seems to be a fairly common use case in our projects as well.

Filters are gone for good (or can it?)

I personally have never used filters in my projects since Vue 2.0, so for me removing it doesn't hurt.

@Hanks10100
Copy link

Hanks10100 commented Sep 22, 2018

Really be excited with Vue 3, and looking forward to it.

Weex is built on bottom of Vue.js, so the breaking changes of syntax will not affect Weex actually. But some compatibility work still can't be omitted, mostly for the new package structure and VNode, not syntax.

VNodes are now context-free

By the way, I think it is a good idea and should be insisted. I wish the Component and VNode could be separate, the interactive API between them could be explicit and minimal.

If be more radical, the vdom (or VNode) may not be needed for native-rendering and server-side-rendering scenarios, at least it should not be handled by javascript. For Weex, it's feasible to implement the vdom (VNode) natively by C++, and expose the corresponding APIs to the running context. Moreover, the create/diff process of vdom can also be compiled to WebAssembly, although it may not certainly improve the performance since WebAssembly can't assess DOM API yet, it can be used to generate HTML strings in the server side. However, if component and vnode have so many coupled properties or features, it would be very hard to make the rendering process to take advantage of native platform abilities.

So, I think separate template, component, and vdom is good for long-term evolving, even if it hurts.

@LinusBorg
Copy link
Member

LinusBorg commented Sep 23, 2018

This looks a amazing, I finally feel like this is something I can navigate and undestand :-P

Points I want to comment on:

Props

Like others I'm not too sure about the whole droppinf of $attrs and $listeners, and especially automatic interance of attributes.

While $attrs and $listeners can probably be re-implemented in userland pretty easily, the last point could be a point of great pain for people - unless we find a way to easily allow for that to be done in userland as well without requiring to touch every single template in your project?

Filters

I'm torn. Filters are usually formatters that people use appliction-wide. When we drop them, people will be forced to implement them as methods, possibly via extensions to the Vue prototype.

  1. this creates the risk of name conflicts. currency is a create name for a filter or a data/prop. Sure, naming rules can help here, but the current implementation doesn't have this problems as filters live in there own namespace.
  2. if people decide to collect their filters in one object with which they expend the prototype to minimize the riks of the previous point - what to call it? And what about conflicts with filters added by plugins?

I'm not too attached to them, but if we drop them, we need a good guide about how to replace them in a maintainable way.

Slots

Awesome, will be very good for performance to make them lazy I imagine. Even though that change requires changes to manually-written render function, the changes are small and easy (and maybe possible to be automated?)

Vnodes

The new API seems great, slim and easy to parse. and I see how attrs and listeners don't fint in there, but could be re-implemented on the instance as mentioned above.

Of course this means, similar to slots, that manually written render components have to be updated, but unlike slots, the change is a little more work. I could imagine that we provide a little helper method that people can wrap their manually written VNodeData objects in to be converted to the new format. That would allow for a quick fix, and can be cleaned manually later.

Mixins

This is a point that wasn't mentioned at all in the OP, and I can't find anything about them in the source either.

I hope they're not killed like React did when they switched to a class-based syntax. So much of the ecosystem relies on them (most of Vuetify is implemented as mixins I think), so I can imagine it would be a big problem.

Would it be possible to make them a static property on the class that is used by the renderer to apply the mixin after creating the instance? what about beforeCreate mixins, then? How do we make them works with types? I have no idea. :/

And on a lighter note: Not sure how to feel about the fact that Vue 3 will have Portals, which kills the need for my only popular OS library :D :D

@KaelWD
Copy link
Contributor

KaelWD commented Sep 23, 2018

How do we make them works with types?

I wrote a little helper for that, could probably be included with vue depending on what the API ends up looking like.

export default mixins(A, B, C).extend({...})
// OR
export default class MyComponent extends mixins(A, B, C) {...}

@octref
Copy link
Member

octref commented Sep 23, 2018

The only thing affecting Vetur is removal of filter. This is great because I can treat interpolations as JS statements without any custom syntax handling. But also +1 to what @LinusBorg said:

I'm not too attached to them, but if we drop them, we need a good guide about how to replace them in a maintainable way.

I might be able to provide editor support that:

  • If Vue 3.0 is observed in package.json
  • Marks filters usage as error
  • Possibly provide auto-fix, if we could programmatically convert the old filter code to a new code

@yyx990803
Copy link
Member Author

yyx990803 commented Sep 23, 2018

Answering a few concerns:

$attrs

First, class and style merging behavior remains the same (and now applies to single-root functional components as well).

Second, I think the ability to render arbitrary attributes on the root of a child component is a useful one, but currently it's a very implicit behavior.

The problem I see with the implicit fall-through is that you read the code of a component being passed props, you won't know which ones will be treated as props and which ones will be treated as attributes without knowing what props the component declares:

<!-- is label a prop or an attribute? -->
<Foo label="123" />

In addition, because props declarations are now optional, we actually don't have a way to implicitly extract attributes for component that does not declare props.

Maybe we can differentiate the two (component props vs. native attributes) similar to how we differentiate component events vs. native events with the .attr modifier:

<!-- this is always a prop, although the component *may* explicitly render it as an attribute, there's no guarantee -->
<Foo label="123" />

<!-- this is always an attribute on the component root -->
<Foo label.attr="123" />

In the compiled code, props with .attr modifiers are extracted as:

h(Comp, {
  // explicitly merged on to child component root, like `class` and `style`.
  attrs: { /* ...* / }
})

Then, we need to consider the case where the component may return a Fragment, or may have a wrapper element and want to take full control of the full-through. In such case, inheritAttrs: false will disable merging for class, style and attrs, and the component author will be able to spread these onto desired element via this.$props.[class | style | attrs]. (this avoids the runtime cost of extracting and allocating memory for $attrs.)

Or, maybe I'm overthinking all this and implicit fall-through is fine (and actually useful). Although, note that the following are orthogonal to whether we keep implicit fall-through or not:

  1. The addition of the .attr modifier. This allows you to be explicit and not relying on anything implicit.
  2. inheritAttrs: false affecting style and class: probably a good idea to make it more consistent.
  3. Exposing parent class/style/attrs on $props when inheritAttrs: false: this allows components to achieve the equivalent of {...props} with v-bind="$props".

The only difference is that with implicit fall-through, any props not declared (plus ones with .attr modifier) will be grouped under $props.attrs. Otherwise, only ones with .attr modifier will be grouped in there.

Thoughts?

Accessing component instance in directives

This is still possible. The vnodes are context-free, but directives are applied in render functions with the component this passed in.

Also re @Justineo : the directive change only affects render functions. Template syntax remains the same.

.pure

Functional components will always update by default due to possible props mutations in a nested property, so using shallow compare by default will be unsafe. Explicitly marking a functional component with .pure = true essentially enables automatic shallow equal shouldUpdate checks.

@yyx990803
Copy link
Member Author

Btw @octref - I'd like the 3.0 compiler to provide infrastructure for even better IDE support - e.g. template type checking / type-aware completions. Maybe even keep the language service in the repo.

@yyx990803
Copy link
Member Author

@KaelWD that mixins helper is great, I was actually still thinking about how to deal with it 😂

@znck
Copy link
Member

znck commented Sep 23, 2018

Adding .attr make me feel Vue templates are not HTML anymore.

@octref
Copy link
Member

octref commented Sep 24, 2018

Btw @octref - I'd like the 3.0 compiler to provide infrastructure for even better IDE support - e.g. template type checking / type-aware completions. Maybe even keep the language service in the repo.

That's great. I'll take a look of the parser / compiler and let you know what change would it take. If we have Error Tolerance in the core parser I should be able to use it in Vetur.

@Justineo
Copy link
Member

<!-- is label a prop or an attribute? -->
<Foo label="123" />

I think it's implemention detail and should be encapsulated by component authors. As I understand, the separation of content attributes (attrs) and IDL attributes (props) only makes sense for native elements, as we can only specify string values in HTML. While Vue templates can specify any data type with v-bind, we don't need to separate them at least for components API. Component users shouldn't care if a documented prop serves as an attribute or not.

In addition, because props declarations are now optional, we actually don't have a way to implicitly extract attributes for component that does not declare props.

Actually because props declarations are optional so we cannot tell the true semantics of a prop. It feels like you define a function without defining parameter types, but pass in types at each time you call a function and this is kinda weird. I'd rather explicitly define props by component authors instead of let users dig into details.

@Akryum
Copy link
Member

Akryum commented Sep 24, 2018

Will there be an easier way for devtools to access to functional components if we cannot access context of vnodes anymore?

@yyx990803
Copy link
Member Author

Update: attribute fallthrough will be preserved when the component has declared props (the behavior will be the same as before). In addition, all parent class, style and nativeOn bindings will be in $attrs, so when the child component returns a Fragment, or has inheritAttrs: false, simply doing v-bind="$attrs" or {...this.$attrs} in JSX will place all non-props bindings from parent root node on that node instead.

@rigor789
Copy link
Contributor

Thanks for inviting me to this early (and exciting) preview of Vue 3.0!

VNodes

Does the change in VNodes affect the template compiler modules? In NativeScript-Vue we have some syntactic sugar that is handled by the template compiler through modules, I'm guessing these will need to be updated (not a deal breaker, just wondering)

Component in Render Functions

No longer resolves component by string names

This is not a deal breaker, but currently we use render: h => h('frame', [h(App)]) in the default nativescript-vue template (frame is a component), as some IDE's complain about the <Frame> element, and mess up autocompletion (phpstorm/webstorm I'm looking at you...)

In templates, components should be uppercase to differentiate from normal elements.

Does uppercase mean <FOOCOMPONENT> or can we still use PascalCase <FooComponent>?

Slots

Scoped slots and normal slots are now unified. There's no more difference between the two. Inside a component, all slots on this.$slots will be functions and all them can be passed arguments.

Will the template syntax for scoped slots change due to this, or is this just an implementation detail/render function specific change?

Filters

I'm not attached to them, but I agree with @LinusBorg about the potential risks of name conflicts.

I haven't had the time to dig through the codebase entirely but just glancing at some parts of it, I'm really liking the new structure, seems a lot easier to follow / contribute to!

@Jinjiang
Copy link
Member

Jinjiang commented Sep 28, 2018

Maybe we can differentiate the two (component props vs. native attributes) similar to how we differentiate component events vs. native events with the .attr modifier:

<!-- this is always a prop, although the component *may* explicitly render it as an attribute, there's no guarantee -->
<Foo label="123" />

<!-- this is always an attribute on the component root -->
<Foo label.attr="123" />

How about <Foo :attrs="{ label: '123'}"> which is more friendly if you want to put a set of aria-* attributes in. And it's not necessary to be converted into { attrs } for render function again.

@DanielRosenwasser
Copy link
Contributor

Component no longer need to delcare props in order to receive props. Everything passed from parent vnode's data (with the exception of internal properties, i,e. key, ref, slots and nativeOn*) will be available in this.$props and also as the first argument of the render function. This eliminates the need for this.$attrs and this.$listeners.

When no props are declared on a component, props will not be proxied on the component instance and can only be accessed via this.$props or the props argument in render functions.

I've read the source but I'm not sure if things are complete yet, but here's my understanding of that:

  1. props always will appear on this.$props or props in a functional component.
  2. If a user specifies props for a new component's options, props will also be available on this.
  3. props that get transferred onto this are all reactive and can be mutated, but are otherwise considered immutable.

Is that correct? If not, can you give an example of the two scenarios in action just so I can get an idea of what the workflow is?

@yyx990803
Copy link
Member Author

yyx990803 commented Oct 2, 2018

@DanielRosenwasser it's definitely not complete yet.

  1. In a functional component there's no this, so the only way to access its props is via the argument.

  2. Yes.

  3. No, props on this are readonly (both type-wise and implementation-wise).

Here's how a user would specify props types:

interface Data {
  foo: string
}

interface Props {
  bar: number
}

class Foo extends Component<Data, Props> {
  static options = {
    props: {
      bar: { type: Number, default: 123 }
    }
  }

  data () {
    return {
      foo: 'hello' // will be an error if type doesn't match interface
    }
  }

  render (props) {
    // accessing data
    this.foo
    this.$data.foo

    // accessing props
    this.bar
    props.bar
    this.$props.bar
  }
}

A few obvious things to improve here:

  1. If the user provided a Props interface but didn't specify the static props options, the props will still be merged onto this in types but not in implementation.

  2. User has to specify the Props interface for compile-time type checking AND the props options for runtime type checking. It'd be nice if the compile-time types can be inferred from the props options like it does for 2.x, although I haven't figured out how to do that yet.

Note the reason we are designing it like this is because we want to make plain ES usage and TS usage as close as possible. As you can see, an ES2015 version of the above would simply be removing the interfaces and replacing the static options with an assignment.

If we can get a decorator implementation that matches the new stage-2/3 proposal, we can provide decorators that make it simpler:

class Foo extends Component {
  @data foo: string = 'hello'
  @prop bar: number = 123
}

@KaelWD
Copy link
Contributor

KaelWD commented Oct 2, 2018

@prop bar: number = 'baz'

TS2322: Type '"baz"' is not assignable to type 'number'.

@Akryum
Copy link
Member

Akryum commented Oct 9, 2018

Could we define events like we do for props? They would be omitted from $attrs. Also it might help with HOC and typing, for example this.$emit.myEvent('foo') and docs.

@Justineo
Copy link
Member

Justineo commented Oct 9, 2018

IMO component consumers will expect the fall-through behavior. The real difference is whether component author have to spread $attrs manually by default, or they only need to do so when they choose to implement a component as a fragment or other special occasions.

If we prevent the fall-through behavior by default, I will expect there will be less 3rd party libs which allow passing arbitrary attributes to DOM, which may have an impact on a11y and other aspects I mentioned in my initial comment.

If component authors will need to spread $attrs or maintain an “allow list” in most cases, then I’d expect the current behavior (fall through by default) to be preserved.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 9, 2018

If we prevent the fall-through behavior by default, I will expect there will be less 3rd party libs which allow passing arbitrary attributes to DOM

@Justineo I agree with your concerns, but I feel like this is an education issue we can solve with documentation, encouraging people to always define a pass-through with v-bind="$attrs"/{...this.$attrs}. We can even create a strongly-recommended rule in eslint-plugin-vue. And even for libraries that ignore documentation and don't use the ESLint plugin, it's a 1-line, 5-second change per component, meaning a medium-sized lib of 50 components will still only take 5 minutes to submit a PR to. To me, that seems like a pretty reasonable trade-off in exchange for explicitness, making sure that attributes and listeners are always passed to the correct element.

@Justineo
Copy link
Member

Justineo commented Oct 9, 2018

I think it's an education issue in either way. We can also educate users that if you don't explicitly take over $attrs they'll be spread into the root element by default...maybe it's only not so intuitive for users coming from React world...

And the current behavior only fails when component authors didn't do anything to redirect $attrs to the correct element, in which case the explicit method would also fail.

I agree that making small changes to components are trivial. But if we are using a 3rd party component lib we have no guarantee whether/when the PR is gonna be merged and published in a new version, while currently it doesn't require users to ask for permission to spread $attrs and it just works in most cases.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 9, 2018

@Justineo

maybe it's only not so intuitive for users coming from React world...

It's not just React users, unfortunately. Probably most new Vue developers I work with are also surprised by this behavior.

And the current behavior only fails when component authors didn't do anything to redirect $attrs to the correct element, in which case the explicit method would also fail.

I'd rather have something work or not work consistently, rather than seeming to work sometimes while very likely to break at any time, even in a patch release, because library authors will accidentally make a breaking change with a different root node or making the root node a fragment, both of which will be common (I know I've been affected by the former with Vue 2). Until the library moves to explicitness, you're essentially using an unstable API.

if we are using a 3rd party component lib we have no guarantee whether/when the PR is gonna be merged and published in a new version

If adding v-bind="$attrs" were a particularly difficult/complex change to review, I think that would be something to consider, but it's not. It's either on the correct element or not, so should be an easy merge. If a library is so unmaintained that not even simple and obvious improvements are being merged, that seems like a separate issue.

@chrisvfritz
Copy link
Contributor

@yyx990803 As an aside, would it be possible to see a kitchen sink example of the proposed class-based component API?

@yyx990803
Copy link
Member Author

@chrisvfritz created an issue for that: #4

@znck
Copy link
Member

znck commented Oct 9, 2018

I'd rather have something work or not work consistently.

I'd prefer a consistent behavior as I don't want to jump into component definition to check if a custom prop can be added or not. It's better to never put extra prop/attrs unless component API/docs explicitly specifies so.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 9, 2018

It's better to never put extra prop/attrs unless component API/docs explicitly specifies so.

@znck For the reasons @Justineo mentioned, I do think there should be an expectation that unless a component renders null or only renders children, it should always pass arbitrary attrs/events to some element. Otherwise, accessibility (e.g. aria-label), e2e testing (e.g. data-testid), etc become much more complicated. Does that make sense?

@Jinjiang
Copy link
Member

Jinjiang commented Oct 9, 2018

I have some different thoughts. 😅

First it's great to support fragment and portal. But at the same time, we break up, at least, weaken the "future ready" feature that Vue components could be built into native web components. Because in 3.0, we are not sure a Vue component has a root DOM element. If we couldn't get a way to build fragment/portal component into native web components someway, I would feel like we leave future web standard a little more far away.

Second, In web platform, I suggest to make global html attributes like style, class, aria-* and native DOM events always meaningful and configurable: that's also what native web components do. And keep the syntax as same as possible. It follows users' intuitions. Maybe we can consider introducing some new symbol/prefix for custom component event.

And a small question: I'm not sure what the render functions will be if I write more than 2 custom directives like <comp v-foo="x" v-bar="y">. Does the order matter?

Thanks.

@yyx990803
Copy link
Member Author

yyx990803 commented Oct 9, 2018

@Jinjiang

Because in 3.0, we are not sure a Vue component has a root DOM element.

Native web components always have its own custom element / shadow root that can serve as the root element, so I don't really think that's a problem.

I suggest to make global html attributes like style, class, aria-* and native DOM events always meaningful and configurable

With the introduction of fragments, we can no longer rely on implicit behavior. I think the solution is making it a best practice to always spread $attrs if you intend to ship your component as a library. (private components are fine because the user can modify them anytime).

And a small question: I'm not sure what the render functions will be if I write more than 2 custom directives like . Does the order matter?

It will look something like this:

const comp = resolveComponent(this, 'comp')
const foo = resolveDirective(this, 'foo')
const bar = resolveDirective(this, 'bar')

return applyDirectives(
  h(comp),
  this,
  [foo, this.x],
  [bar, this.y]
)

@Jinjiang
Copy link
Member

Jinjiang commented Oct 10, 2018

I think the solution is making it a best practice to always spread $attrs if you intend to ship your component as a library.

I guess there would be quite amount of mis-operations when:

  • write a component which is designed to accept style but forget $attrs, or
  • use a class to a fragment component or "closed" (without spreading $attrs) component but see nothing happened without any tips.

Maybe it's better to make $attrs implicit and print warnings when its a fragment/portal/closed component IMO. It helps both authors and users to make less mistakes.

Btw, till now, I sometimes mis-wrote @click.native as @click and get lost for a while. If possible, I wish 3.0 a better experience. 😂

Thanks.

@yyx990803
Copy link
Member Author

write a component which is designed to accept style but forget $attrs

The best/common practice would be always merging $attrs in its entirety. It would be a conscious decision to only allow style falling through so I don't think it's possible to "forget".

use a class to a fragment component or "closed (without spreading $attrs)" component but see nothing happened without any tips.

This we can actually detect and throw a warning for (if $attrs is not empty, but never accessed during render).

@Justineo
Copy link
Member

This we can actually detect and throw a warning for (if $attrs is not empty, but never accessed during render).

What if this is intentional and all allowed attributes are declared in props instead?

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 10, 2018

Maybe it's better to make $attrs implicit and print warnings when its a fragment/portal/closed component IMO.

@Jinjiang I really like the idea of a warning for users, but with any implicit pass-through, I really think we're only delaying pain by allowing users to rely on an unstable API that the component author didn't explicitly define.

Btw, till now, I sometimes mis-wrote @click.native as @click and get lost for a while. If possible, I wish 3.0 a better experience. 😂

@Jinjiang Me too. 😅 But if we remove all implicit pass-through, that problem is solved. 😉

What if this is intentional and all allowed attributes are declared in props instead?

@Justineo Since there will be valid use cases for not binding $attrs (null renders and components that only render children), I think we could provide a neutral warning that doesn't imply the component author definitely did something wrong. Maybe something like:

Component <Foo> was passed attribute "bar", but "bar" was not used. If this is not a typo and the component renders its own markup, it's recommended that the component author spreads arbitrary attributes to some element using v-bind="$attrs" in a template, or {...this.$attrs} in JSX.

What do you think?

@chrisvfritz
Copy link
Contributor

@Justineo Also, I think null and children renders are the only cases where intentionally not spreading $attrs would be valid, but maybe I've missed a use case. Can you think of a situation where a component has its own markup, but the author might not want to spread $attrs?

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 10, 2018

@yyx990803 And actually, is there potentially a way we could detect if a component has generated its own markup, rather than rendering null or just its children? That way, we could have more precise warnings. If a component does generate its own markup, then:

Component <Foo> was passed attribute "bar", but "bar" was not used. Since this component generates its own markup, it's recommended that the component author spreads arbitrary attributes to some element using v-bind="$attrs" in a template, or {...this.$attrs} in JSX.

And if the component doesn't generate its own markup:

Component <Foo> was passed attribute "bar", but "bar" is not a prop. The valid props are:
  - propA (String)
  - propB (Number)
  - propC (Boolean)

What do you think?

@Justineo
Copy link
Member

I'm not sure if there will be such use cases that the author want to hand-pick all allowed attributes and declare them inside props so nothing in $attrs will be touched. Technically it is a valid usage but users may come across the warning.

@chrisvfritz
Copy link
Contributor

I'm not sure if there will be such use cases that the author want to hand-pick all allowed attributes and declare them inside props so nothing in $attrs will be touched. Technically it is a valid usage but users may come across the warning.

@Justineo Beyond the unnecessary clutter this would add to the component definition (especially for the minimum 21 aria attributes every element must accept), wouldn't this also prevent unpredictable attributes from being used (e.g. data-testid for e2e tests)? For that reason, wouldn't hand-picking attributes always be an anti-pattern?

@Justineo
Copy link
Member

wouldn't hand-picking attributes always be an anti-pattern?

Yes. I'm just saying it's valid in the current mechanism, despite being an anti-pattern. On a second thought it may be good enough to suggest them spread $attrs instead.

Another issue might be whether we suggest authors to always spread the $attrs object in the warning message, or give them some space to explicitly allow/disallow the attributes end up with in the target element. I saw some React component libs only allow data-*/aria-*/role to be passed through.

@chrisvfritz
Copy link
Contributor

chrisvfritz commented Oct 11, 2018

@Justineo

suggest authors to always spread the $attrs object in the warning message

That would be my preference, as I'm still not convinced there's a valid use case for whitelisting. I feel like there can always be exceptions where an attribute beyond data-*/aria-*/role is necessary due to a strange requirement in a vendor library or specific accessibility technology.

give them some space to explicitly allow/disallow the attributes end up with in the target element. I saw some React component libs only allow data-/aria-/role to be passed through.

If users really need to do attribute whitelisting (or blacklisting), they technically already can, e.g. with:

pickBy(
  this.$attrs, 
  (value, key) =>
    /^(data|aria)[A-Z]/.test(key) ||
    ['role'].includes(key)
)

And this would still trigger the getter for $attrs, thus disabling the warning, so I think that should keep those users from complaining.

@yyx990803
Copy link
Member Author

As this thread has become super long and hard to navigate, let's open separate issues to discuss specific topics.

@yyx990803
Copy link
Member Author

Open separate issue for attrs fallthrough behavior: #5

@yyx990803
Copy link
Member Author

Closing in favor of public RFCs.

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