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

Attribute Fallthrough + Functional Component Updates #154

Merged
merged 5 commits into from
Apr 16, 2020

Conversation

yyx990803
Copy link
Member

@yyx990803 yyx990803 commented Apr 2, 2020


The above changes are submitted together for review because they are closely related to one another. Below I will try to explain how we arrived at these changes.

The Problem

In an earlier phase of Vue 3 development we introduced [RFC0010] Optional Props Declaration - but this led to an issue with attr fallthrough. Given a component with no props declaration, we are no longer able to distinguish between the following user intentions:

  1. A component utilizing optional props, i.e. "You can pass me any props, and I will decide what to do with them".

  2. A component expecting no props. i.e. "I don't expect any props - anything you pass to me should just fallthrough.

In Vue 2.x, a component without props declaration means (2). With optional props declaration, it will mean (1).

For a type (1) component, it would not make sense to apply the attribute fallthrough behavior, because for a given prop, we cannot tell whether the component intends to treat it as a prop or as a fallthrough attribute:

const Foo = {
  setup(props: { id: string }) {
    // use props.id
    // but we don't want `id` to appear on the root element as an attribute.
  }
}

Initially, we wanted to disable attribute fallthrough for components without props declaration in v3 (as proposed in an early draft of the attribute fallthrough RFC).

However, we noticed that users find it surprising that class fallthrough doesn't work when no props are declared. This is quite different from the expectation coming from Vue 2. In general, it is very common for users to rely on the fallthrough of class and style for layout purposes.

So we are at a dilemma here: we cannot make the fallthrough work the same way for components with optional props, but making it not work at all can lead to both confusion and inconvenience.

Review the Goals

To get a better idea of how to evaluate the trade-off, let's look at the original reason for the fallthrough behavior to exist:

  1. Cross-component layout styling: It is very common for users to rely on the fallthrough of class and style for adjusting layout of nested components. In React, the lack of such a mechanism has led to various different workarounds and debates (e.g. should a component manage its own margins, etc.).

  2. Convenience for component library authors: The automatic fallthrough is valuable when shipping UI components that wraps native elements, so that the author doesn't need to manually proxy all the possible attributes on the native element via props (This is also the reason why we are making v-on fallthrough).

  3. Accessibility control for component consumers: Fallthrough is also useful for adding aria-* attributes to 3rd party components that the user has no control over.

Other considerations:

  1. Optional Props: the current issue is caused by the introduction of optional props. If we remove this feature, then the fallthrough behavior can be kept the same as in 2.x (except for warnings on fragment components).

  2. Consistency: given that we keep optional props, we can choose to:

    1. Make the fallthrough rules consistent for all components regardless of whether it has props declaration or not, OR

    2. Make it behave differently based on the presence of props declaration.

Possible Solutions

A. Removing Optional Props

Removing the optional props feature altogether would allow us to keep the same behavior as in v2. Here we essentially need to compare the benefits of optional props vs. the trade-offs we have to make to keep it. Optional props have the following benefits:

  1. Makes v3 functional components more succinct to use:

    // with optional props
    const Foo = props => h('div', props.msg)
    
    // without
    const Foo = props => h('div', props.msg)
    Foo.props = ['msg']
  2. Makes it possible to use only type-annotations for props in TS, as some users prefer using TS type syntax for more complex prop types:

    const Foo = defineComponent({
      setup(props: { complexObj: Record<string, number> }) {
      }
    })

    With the props declaration it is a bit awkward:

    import { PropType } from 'vue'
    
    const Foo = defineComponent({
      props: {
        complexObj: Object as PropType<Record<string, number>>
      },
      setup(props) {
      }
    })

    However, it could also be argued that this can potentially lead TS components in the ecosystem to exhibit different attribute fallthrough behavior from JS components.

B. Whitelist for All Cases

Assuming we keep optional props, one option is to allow only a whitelist of attributes to fallthrough for all components (regardless of the presence of props declaration). This is what I proposed in #137. However, the issue with this approach is that a minimal whitelist compromises goal 2 (convenience for component authors) and goal 3 (a11y). In order to better support these goals, we have to define a whitelist that becomes complicated and difficult to remember. In addition, the whitelist has possibility to clash with names that the user may intend to use as a prop (e.g. id or role), creating more potential confusions.

C. Whitelist only for Components with Optional Props

Another option is differentiating between a component with no props declaration vs. a component with empty props declaration:

// component with no props declaration: optional props.
const Foo = {
  setup(props) {
    // anything passed to Foo will be in props
  }
}

// component with empty props declaration: no props.
const Foo = {
  props: {},
  setup(props) {
    // props will always be an empty object
  }
}

The upside is we get to keep optional props for stateful components; the downside is that this difference is quite subtle and can be confusing when it bites.

D. Optional Props only for Functional Components

This is the option this PR is going with.

Yet another option is to only support optional props for functional components:

  • All stateful components must still explicitly declare props.

    • This makes attribute fallthrough largely the same as v2.
  • Functional components without props declaration will have everything passed in as props. It will only have implicit fallthrough for class, style and v-on listeners.

    • Plain functions as Functional Component is new in v3, so it is not going to affect existing code as much.

    • These functional components are typically created in very situational / one-off use cases where full fallthrough typically isn't needed. class, style and v-on should cover most of the use cases.

@yyx990803 yyx990803 added 3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core labels Apr 2, 2020
Comment on lines 56 to 58
- In v3, the `@click` listener will fallthrough and register a native click listener on the root of `MyButton`. This means component authors no longer need to proxy native events to custom events in order to support `v-on` usage without the `.native` modifier. In fact, the `.native` modifier will be removed altogether.

Note this may result in unnecessary registration of native event listeners when the user is only listening to component custom events, which we discuss below in [Unresolved Questions](#unresolved-questions).
Copy link
Contributor

@CyberAP CyberAP Apr 2, 2020

Choose a reason for hiding this comment

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

Given MyButton is authored that way would it produce 2 click events with implicit event listeners fallthrough?

<button @click="$emit('click', $event)">Click me</button>

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, the whole point is you don't proxy events anymore.

Comment on lines 218 to 220
When spreading `$attrs` with `v-bind`, all parent listeners are applied to the target element as native DOM listeners. The problem is that these same listeners can also be triggered by custom events - in the above example, both a native click event and a custom one emitted by `this.$emit('click')` in the child will trigger the parent's `foo` handler. This may lead to unwanted behavior.

Props do not suffer from this problem because declared props are removed from `$attrs`. Therefore we should have a similar way to "declare" emitted events from a component. Event listeners for explicitly declared events will be removed from `$attrs` and can only be triggered by custom events emitted by the component via `this.$emit`. There is currently [an open RFC for it](https://github.com/vuejs/rfcs/pull/16) by @niko278. It is complementary to this RFC but does not affect the design of this RFC, so we can leave it for consideration at a later stage, even after Vue 3 release.
Copy link
Contributor

@CyberAP CyberAP Apr 2, 2020

Choose a reason for hiding this comment

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

To support just style and class attributes (without event listeners) on the component root it won't be safe to simply reply on implicit fallthrough because:

  1. All components have a fallthrough for style, class and v-on attributes
  2. Given many of the components emit their custom events we'll assign unnecessary event handlers on their roots from implicit fallthrough
  3. To opt-out of this behaviour we declare inheritAttrs: false for each component that has a custom event
  4. Now to support style and class attributes on the root of those components we write v-bind="filteredAttrs" which contains needed attributes only

Compare that with a v2 behaviour where nothing is required to get this behaviour out of the box and for arbitrary attributes only v-bind="$attrs" is required. Also no extra work is required to make custom events work safely (that may have clashing names with native events).

Even if #16 does become implemented there're too many workarounds required for just custom events to be able to safely work.


I can hardly imagine $attrs containing event listeners to be as useful as $attrs in v2.

Consider this example:

<input @input="$emit('input', $event)" v-bind="$attrs">

If you do this in v3 then you should get 2 input events at once. With inheritAttrs: true this should produce 3 events since we also have implicit fallthrough.

In each of those scenarios you should always filter $attrs and I couldn't find a good use-case where you need both attributes and listeners in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly I don't understand your concerns, so I'll just answer with what I think might address it:

  1. In v2 the fallthrough applies to all attributes. It's not just class and style. You never needed v-bind="$attrs" unless you use inheritAttrs: false. However, because v-on does not automatically fallthrough, users have to manually proxy the native events to custom events.

  2. In v3 with the v-on fallthrough you should never need to explicitly proxy events again. So in most cases implicit fallthrough will just work and you don't need to do anything.

  3. The only downside is when users listen for a custom event, a native listener is also added. This is why emits is suggested here: by declaring the custom events the component intends to emit, Vue can avoid binding them as native listeners.

So the only extra thing you need is the emits option, which can serve documentation and runtime validation purpose as well, just like props.

Copy link
Contributor

Choose a reason for hiding this comment

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

However, because v-on does not automatically fallthrough, users have to manually proxy the native events to custom events.

Having to add a .native modifier is way easier than dealing with filtering $attrs when authoring components. Being able to control whether an event listener should be assigned to the root or not on a component client level looks like a more reasonable approach for me, at least it does solve the issue of event listeners implicit fallthrough.

The naming could probably be reconsidered though. I suggest leaving the modifier behaviour but calling it .root rather than .native (that would better communicate that the listener would be assigned to a component's root and a fragment vnode does not fulfill this requirement).


The problem with this approach is that it simplifies things if you need your listeners on the root element and complicates things if you need them somewhere else. It also complicates things if you have root event listeners emitting anything but the event object, in that case you'll have to disable fallthrough behaviour completely and manually filter $attrs object.

So some components will benefit from that and others will require more work than usual. Vue 2 does not have that problem.

Maybe a balance could be achieved here?

Consider a component that has to have implicit attributes fallthrough, but at the same time emit a custom event:

<input @input="$emit('input', $event.target.value)" v-bind="filteredAttrs">

<script>
  export default {
    inheritAttrs: false,
    computed: {
      filteredAttrs() {...}
    }
  }
</script>

The component above should benefit from attributes fallthrough in theory but it has to disable that behaviour in order to have a custom event. filteredAttrs should now filter all attributes except style, class and onInput listener.

Since #16 is not part of this RFC at the current state this is how I imagine it's supposed to work for this case when this RFC is accepted. If the new behaviour requires emits to deal with the issues above I think it should be a part of this RFC.

Copy link
Member

Choose a reason for hiding this comment

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

Getting rid of .native is a very good thing imo

Copy link
Member Author

Choose a reason for hiding this comment

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

So, the primary concern is for components that emit custom events with the same names as native events? How often does that really happen?

For most components that are wrappers of native equivalents (e.g. <MyButton>, <MyInput>), users would expect native event names to behave like native listeners. And for input components, you really should support v-model as the primary usage anyway, so I believe the cases where you need to emit a custom event named input AND the root element happens to be an <input> element is quite rare.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my codebase this happens a lot. This change would stop me from migrating to v3. Also there is not a single case of .native in my project, but for some reason I'll be forced to go through a migration step in order to remove it.

Copy link
Member Author

@yyx990803 yyx990803 Apr 3, 2020

Choose a reason for hiding this comment

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

@CyberAP assuming emits option is added, you can explicit control the events that you want to "take over". E.g. with emits: ['input'], @input will no longer fallthrough as a native listener and you can decide what to do with it, with any other event listeners still automatically fallthrough. If you use inheritAttrs: true, listeners listed in emits are also removed from $attrs. Does that resolve your problem?

Copy link
Contributor

Choose a reason for hiding this comment

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

That does resolve my problem provided that this migration step is not manual.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically a codemod can scan all instances of $emit calls in a component and generate the emits option for you.

@leopiccionia
Copy link

In Vue 2, trying to declare a prop called class or style throws a message like the following, that avoids turning off the fallthrough of those attributes:

[Vue warn]: "style" is a reserved attribute and cannot be used as component prop.

If this limitation was removed, would it be possible to declare a prop named onInput to avoid both the implicit fallthrough of the @input listener and its presence in $attrs? Considering that v-bind handle them specially, perhaps it's not possible or desirable.

If it was possible, it would allow resolving both name clashes between native and custom events, and the unresolved question about avoiding certain native listeners.

Perhaps declaring a fallthrough attribute as a prop could be a development-time warning instead of error.

@yyx990803
Copy link
Member Author

yyx990803 commented Apr 3, 2020

@leopiccionia in fact that already works in v3, although I think a dedicated emits option is more explicit (and it is easier to work with for IDEs and analyzers).

yyx990803 added a commit to vuejs/core that referenced this pull request Apr 3, 2020
BREAKING CHANGE: attribute fallthrough behavior has been adjusted
according to vuejs/rfcs#154
### Multiple Root / Fragment Components

In Vue 3, components can have multiple root elements (i.e. fragment root). In such cases, an automatic merge cannot be performed. The user will be responsible for spreading the attrs to the desired element:

Copy link

@tylerkrupicka tylerkrupicka Apr 3, 2020

Choose a reason for hiding this comment

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

Sorry if this is an obvious question, with this proposal is there a way to manually indicate what native HTML element is represented by the component?

For example, when building a component in React I can specify (in TypeScript) that my component extends JSX.IntrinsicElements<'button'>, which then gives all consumers of my component proper autocomplete for HTML button attributes + my custom props.

I see here that Vue 3 will try to automatically do pass through, but won't if there's a fragmented root. There are also cases where you could have a wrapper div around a native button, but still want to spread and have prop validation for the underlying button.

Copy link
Member Author

@yyx990803 yyx990803 Apr 3, 2020

Choose a reason for hiding this comment

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

This seems to be two separate questions.

For TSX inference, it is possible but not as straightforward:

const MyComponent = defineComponent({
  props: {
    foo: String
  }
})

const MyComponentWithButtonProps = MyComponent as {
  new(): InstanceType<typeof MyComponent> & { $props: JSX.IntrinsicElements['a'] }
}

// voila
<MyComponentWithButtonProps href="foo" />

A helper type can be used to make this cleaner:

type ExtendProps<Comp extends { new(): any }, elements extends string> = {
  new(): InstanceType<Comp> & { $props: JSX.IntrinsicElements[elements] }
}

const MyCompWithButtonProps = MyComponent as ExtendProps<typeof MyComponent, 'a'> // you can even use a union type to extend multiple elements

For fragment root / wrapper elements, you can always manually control where the $attrs should be applied to. It's discussed right before this section you are commenting on.

Copy link

@tylerkrupicka tylerkrupicka Apr 3, 2020

Choose a reason for hiding this comment

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

Yeah I got that you could manually place the attrs anywhere, I just wanted to make sure you could indicate in the types which element you are applying them to. Thanks!

@yyx990803
Copy link
Member Author

This RFC is now in final comments stage. An RFC in final comments stage means that:

The core team has reviewed the feedback and reached consensus about the general direction of the RFC and believe that this RFC is a worthwhile addition to the framework.
Final comments stage does not mean the RFC's design details are final - we may still tweak the details as we implement it and discover new technical insights or constraints. It may even be further adjusted based on user feedback after it lands in an alpha/beta release.
If no major objections with solid supporting arguments have been presented after a week, the RFC will be merged and become an active RFC.

@yyx990803 yyx990803 added the final comments This RFC is in final comments period label Apr 9, 2020
@CyberAP
Copy link
Contributor

CyberAP commented Apr 9, 2020

@yyx990803 if event listeners are excluded from $attrs when they are listed in emits what would be the proper way to check for their presence? Should it be $props.onClick?

@yyx990803
Copy link
Member Author

yyx990803 commented Apr 9, 2020

@CyberAP $emit calls return an array of the return values of triggered handlers. If no handler was found, it would return an empty array, so you can check its length to know if it triggered any handlers. If you want to check for the presence without actually emitting, then you'll have to check it on $vnode.props, which is the raw props passed to the component, without any props/emits filtering.

Edit: emit returning values have been removed as of latest alpha (see discussion in #16).

$emit by definition should be fire and forget. If you do care about a listener's presence, you can do so by declaring it as a prop (onXXX) instead. This allows you to make it required or check its presence before calling it.

@backbone87
Copy link

i would expect having 3 different dictionaries:

  • props: Record<string, unknown>: everything declared as prop
  • listeners: Record<string, Listener[]>: everything declared as emits
  • attrs: Record<string, unknown>: everything else

@Akryum
Copy link
Member

Akryum commented Apr 13, 2020

@yyx990803 Now that optional props are gone, could we think about changing props to an object of Ref?

@yyx990803
Copy link
Member Author

@Akryum I don't think so. That also should be in its own RFC if it were to be discussed.

@yyx990803
Copy link
Member Author

@backbone87 I don't know what you'd need listeners for in this case, since you are supposed to trigger them by calling emit instead of directly calling them. Since they are declared as the component's own events, there's no point in passing them down or binding them in templates. If you want to have access to them as normal functions then it's better to just declare them as onXXX props.

@backbone87
Copy link

backbone87 commented Apr 15, 2020

@yyx990803 the usecase is to check if a listener was attached. consider the following template:

<span>Some Label</span>
<a v-if="$listeners.onMoreClick" @click="$emit('moreClick')">more...</a>

edit:
ofc this can also be done via an additional prop, but since the vue listeners is a direct channel from child to parent, not having a listener set by the parent means noone could listen to the emit and therefore the element triggering the event may not be rendered at all.

edit2:
i dont need the listeners dict directly, but a way to check for "was a listener for emit named X attached?" would be nice.

@yyx990803
Copy link
Member Author

yyx990803 commented Apr 16, 2020

@backbone87 you can also declare the onMoreClick prop in addition to emits so that it is present as a prop - which allows you just check v-if="onMoreclick". Or if you don't want to declare both, you can check it from v-if="$vnode.props.onMoreClick".

@yyx990803 yyx990803 merged commit 49de6de into master Apr 16, 2020
@backbone87
Copy link

$vnode.props.onMoreClick will do, but has a little bit of a "hackish" flavor for general usage. this isnt a problem for experienced vue developers, but in a team with changing members i would usually refrain from using such "hidden gems".

@CyberAP
Copy link
Contributor

CyberAP commented Apr 16, 2020

$vnode.props.onMoreClick will do, but has a little bit of a "hackish" flavor for general usage. this isnt a problem for experienced vue developers, but in a team with changing members i would usually refrain from using such "hidden gems".

That's what I thought at first as well, but the times when you actually need to do so is negligible.

@aztalbot
Copy link

aztalbot commented Apr 16, 2020

Just want to point out one caveat with relying on the approach of adding onMoreClick to props: if someone didn't read this bit of the documentation and tried to listen via @more-click, the "more ..." button would be rendered because props.onMoreClick would be populated with the handlers, even though $emit('moreClick') will not trigger that handler (props case transformation vs. no event case transformation). So the vnode props approach seems good. This seems like it would all be very rare, and people should read the docs anyway, just pointing this out because maybe mixing emits and onEvent props should be discouraged as it might lead to confusion around the prop being populated but emit having no effect. It's maybe just a small caveat to document. (side note: for the vnode props approach, I got _ctx.$vnode is undefined and had to do $.vnode.props using the webpack preview with alpha 13).


following up ... from the docs:

Unlike components and props, event names will never be used as variable or property names in JavaScript, so there’s no reason to use camelCase or PascalCase.

now that we have emits, this isn't really true anymore 🤔


It feels like putting events in emits and also having onEvent props now might make camelCase events more common, where it was previously discouraged. Is there any reason why camelCase events shouldn't trigger kebab-case equivalents? There is logic that already does this for update:-prefixed events. For DOM templates, users would have to use kebab-case, and this would ensure emitted camelCase events would still trigger those.

@xovel
Copy link

xovel commented May 12, 2020

... decalration is a typo.

@yyx990803 yyx990803 removed the final comments This RFC is in final comments period label Jul 1, 2020
@bencodezen bencodezen mentioned this pull request Jul 6, 2020
25 tasks
@a1mersnow
Copy link

So what about custom directives?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.x This RFC only targets 3.0 and above breaking change This RFC contains breaking changes or deprecations of old API. core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants