From e1660f4338fbf4d2a434e13193a58e00f844379b Mon Sep 17 00:00:00 2001 From: Evan You Date: Fri, 28 Feb 2020 17:53:26 -0500 Subject: [PATCH] refactor(runtime-core): adjust attr fallthrough behavior BREAKING CHANGE: adjust attr fallthrough behavior Updated per pending RFC https://github.com/vuejs/rfcs/pull/137 - Implicit fallthrough now by default only applies for a whitelist of attributes (class, style, event listeners, a11y attributes, and data attributes). - Fallthrough is now applied regardless of whether the component has explicitly declared props. (close #749) --- .../rendererAttrsFallthrough.spec.ts | 102 ++++-------------- packages/runtime-core/src/componentProps.ts | 2 +- .../runtime-core/src/componentRenderUtils.ts | 30 ++++-- packages/runtime-core/src/vnode.ts | 13 ++- 4 files changed, 52 insertions(+), 95 deletions(-) diff --git a/packages/runtime-core/__tests__/rendererAttrsFallthrough.spec.ts b/packages/runtime-core/__tests__/rendererAttrsFallthrough.spec.ts index 6ee03207477..0f5f54dcdf2 100644 --- a/packages/runtime-core/__tests__/rendererAttrsFallthrough.spec.ts +++ b/packages/runtime-core/__tests__/rendererAttrsFallthrough.spec.ts @@ -15,7 +15,7 @@ import { mockWarn } from '@vue/shared' describe('attribute fallthrough', () => { mockWarn() - it('everything should be in props when component has no declared props', async () => { + it('should allow whitelisted attrs to fallthrough', async () => { const click = jest.fn() const childUpdated = jest.fn() @@ -42,87 +42,19 @@ describe('attribute fallthrough', () => { const Child = { setup(props: any) { - onUpdated(childUpdated) - return () => - h( - 'div', - mergeProps( - { - class: 'c2', - style: { fontWeight: 'bold' } - }, - props - ), - props.foo - ) - } - } - - const root = document.createElement('div') - document.body.appendChild(root) - render(h(Hello), root) - - const node = root.children[0] as HTMLElement - - expect(node.getAttribute('id')).toBe('test') - expect(node.getAttribute('foo')).toBe('1') - expect(node.getAttribute('class')).toBe('c2 c0') - expect(node.style.color).toBe('green') - expect(node.style.fontWeight).toBe('bold') - expect(node.dataset.id).toBe('1') - node.dispatchEvent(new CustomEvent('click')) - expect(click).toHaveBeenCalled() - - await nextTick() - expect(childUpdated).toHaveBeenCalled() - expect(node.getAttribute('id')).toBe('test') - expect(node.getAttribute('foo')).toBe('1') - expect(node.getAttribute('class')).toBe('c2 c1') - expect(node.style.color).toBe('red') - expect(node.style.fontWeight).toBe('bold') - }) - - it('should implicitly fallthrough on single root nodes', async () => { - const click = jest.fn() - const childUpdated = jest.fn() - - const Hello = { - setup() { - const count = ref(0) - - function inc() { - count.value++ - click() - } - - return () => - h(Child, { - foo: 1, - id: 'test', - class: 'c' + count.value, - style: { color: count.value ? 'red' : 'green' }, - onClick: inc - }) - } - } - - const Child = defineComponent({ - props: { - foo: Number - }, - setup(props) { onUpdated(childUpdated) return () => h( 'div', { + id: props.id, // id is not whitelisted class: 'c2', style: { fontWeight: 'bold' } }, props.foo ) } - }) + } const root = document.createElement('div') document.body.appendChild(root) @@ -130,25 +62,22 @@ describe('attribute fallthrough', () => { const node = root.children[0] as HTMLElement - // with declared props, any parent attr that isn't a prop falls through - expect(node.getAttribute('id')).toBe('test') + expect(node.getAttribute('id')).toBe('test') // id is not whitelisted, but explicitly bound + expect(node.getAttribute('foo')).toBe(null) // foo is not whitelisted expect(node.getAttribute('class')).toBe('c2 c0') expect(node.style.color).toBe('green') expect(node.style.fontWeight).toBe('bold') + expect(node.dataset.id).toBe('1') node.dispatchEvent(new CustomEvent('click')) expect(click).toHaveBeenCalled() - // ...while declared ones remain props - expect(node.hasAttribute('foo')).toBe(false) - await nextTick() expect(childUpdated).toHaveBeenCalled() expect(node.getAttribute('id')).toBe('test') + expect(node.getAttribute('foo')).toBe(null) expect(node.getAttribute('class')).toBe('c2 c1') expect(node.style.color).toBe('red') expect(node.style.fontWeight).toBe('bold') - - expect(node.hasAttribute('foo')).toBe(false) }) it('should fallthrough for nested components', async () => { @@ -179,12 +108,16 @@ describe('attribute fallthrough', () => { const Child = { setup(props: any) { onUpdated(childUpdated) + // HOC simply passing props down. + // this will result in merging the same attrs, but should be deduped by + // `mergeProps`. return () => h(GrandChild, props) } } const GrandChild = defineComponent({ props: { + id: String, foo: Number }, setup(props) { @@ -193,6 +126,7 @@ describe('attribute fallthrough', () => { h( 'div', { + id: props.id, class: 'c2', style: { fontWeight: 'bold' } }, @@ -351,11 +285,11 @@ describe('attribute fallthrough', () => { // #677 it('should update merged dynamic attrs on optimized child root', async () => { - const id = ref('foo') + const aria = ref('true') const cls = ref('bar') const Parent = { render() { - return h(Child, { id: id.value, class: cls.value }) + return h(Child, { 'aria-hidden': aria.value, class: cls.value }) } } @@ -370,14 +304,14 @@ describe('attribute fallthrough', () => { document.body.appendChild(root) render(h(Parent), root) - expect(root.innerHTML).toBe(`
`) + expect(root.innerHTML).toBe(``) - id.value = 'fooo' + aria.value = 'false' await nextTick() - expect(root.innerHTML).toBe(`
`) + expect(root.innerHTML).toBe(`
`) cls.value = 'barr' await nextTick() - expect(root.innerHTML).toBe(`
`) + expect(root.innerHTML).toBe(`
`) }) }) diff --git a/packages/runtime-core/src/componentProps.ts b/packages/runtime-core/src/componentProps.ts index a0d1b8046b1..61b8122e8cf 100644 --- a/packages/runtime-core/src/componentProps.ts +++ b/packages/runtime-core/src/componentProps.ts @@ -214,7 +214,7 @@ export function resolveProps( lock() instance.props = props - instance.attrs = options ? attrs || EMPTY_OBJ : props + instance.attrs = attrs || EMPTY_OBJ instance.vnodeHooks = vnodeHooks || EMPTY_OBJ } diff --git a/packages/runtime-core/src/componentRenderUtils.ts b/packages/runtime-core/src/componentRenderUtils.ts index 5963f5b303c..f5628792daa 100644 --- a/packages/runtime-core/src/componentRenderUtils.ts +++ b/packages/runtime-core/src/componentRenderUtils.ts @@ -11,7 +11,7 @@ import { cloneVNode } from './vnode' import { handleError, ErrorCodes } from './errorHandling' -import { PatchFlags, ShapeFlags, EMPTY_OBJ } from '@vue/shared' +import { PatchFlags, ShapeFlags, EMPTY_OBJ, isOn } from '@vue/shared' import { warn } from './warning' // mark the current rendering instance for asset resolution (e.g. @@ -79,17 +79,17 @@ export function renderComponentRoot( } // attr merging + let fallthroughAttrs if ( - Component.props != null && Component.inheritAttrs !== false && attrs !== EMPTY_OBJ && - Object.keys(attrs).length + (fallthroughAttrs = getFallthroughAttrs(attrs)) ) { if ( result.shapeFlag & ShapeFlags.ELEMENT || result.shapeFlag & ShapeFlags.COMPONENT ) { - result = cloneVNode(result, attrs) + result = cloneVNode(result, fallthroughAttrs) // If the child root node is a compiler optimized vnode, make sure it // force update full props to account for the merged attrs. if (result.dynamicChildren !== null) { @@ -97,7 +97,8 @@ export function renderComponentRoot( } } else if (__DEV__ && !accessedAttrs && result.type !== Comment) { warn( - `Extraneous non-props attributes (${Object.keys(attrs).join(',')}) ` + + `Extraneous non-props attributes (` + + `${Object.keys(attrs).join(', ')}) ` + `were passed to component but could not be automatically inherited ` + `because component renders fragment or text root nodes.` ) @@ -142,7 +143,24 @@ export function renderComponentRoot( return result } -function isElementRoot(vnode: VNode) { +const getFallthroughAttrs = (attrs: Data): Data | undefined => { + let res: Data | undefined + for (const key in attrs) { + if ( + key === 'class' || + key === 'style' || + key === 'role' || + isOn(key) || + key.indexOf('aria-') === 0 || + key.indexOf('data-') === 0 + ) { + ;(res || (res = {}))[key] = attrs[key] + } + } + return res +} + +const isElementRoot = (vnode: VNode) => { return ( vnode.shapeFlag & ShapeFlags.COMPONENT || vnode.shapeFlag & ShapeFlags.ELEMENT || diff --git a/packages/runtime-core/src/vnode.ts b/packages/runtime-core/src/vnode.ts index 7b415ddbc67..568052a4b07 100644 --- a/packages/runtime-core/src/vnode.ts +++ b/packages/runtime-core/src/vnode.ts @@ -399,15 +399,20 @@ export function mergeProps(...args: (Data & VNodeProps)[]) { const toMerge = args[i] for (const key in toMerge) { if (key === 'class') { - ret.class = normalizeClass([ret.class, toMerge.class]) + if (ret.class !== toMerge.class) { + ret.class = normalizeClass([ret.class, toMerge.class]) + } } else if (key === 'style') { ret.style = normalizeStyle([ret.style, toMerge.style]) } else if (handlersRE.test(key)) { // on*, vnode* const existing = ret[key] - ret[key] = existing - ? [].concat(existing as any, toMerge[key] as any) - : toMerge[key] + const incoming = toMerge[key] + if (existing !== incoming) { + ret[key] = existing + ? [].concat(existing as any, toMerge[key] as any) + : incoming + } } else { ret[key] = toMerge[key] }