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.

Sign up for GitHub

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

fix(runtime-dom): map kebab attributes to camelCase props (fix #13055) #13085

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

fix(runtime-dom): map kebab attributes to camelCase props (fix #13055) #13085

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9cb07e6
fix(runtime-dom): check for camel-case element prop
vonBrax Mar 21, 2025
5b21129
test: add kebab to camel prop tests
vonBrax Mar 21, 2025
4b473d2
Merge branch 'main' into main
edison1105 Mar 24, 2025
489d285
perf(runtime-dom): cache element prop key
vonBrax Mar 25, 2025
2e8c775
Merge branch 'main' of github-private:vonBrax/vuejs-core
vonBrax Mar 25, 2025
b4bdb6d
Merge branch 'vuejs:main' into main
vonBrax Mar 25, 2025
d639feb
Revert "perf(runtime-dom): cache element prop key"
vonBrax Mar 27, 2025
74d74d7
chore: Merge branch 'main' of github-private:vonBrax/vuejs-core
vonBrax Mar 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions packages/runtime-dom/__tests__/patchProps.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import {
vModelCheckbox,
withDirectives,
} from '../src'
import { defineComponent } from '@vue/runtime-test'
import { render as domRender } from 'vue'

describe('runtime-dom: props patching', () => {
test('basic', () => {
Expand Down Expand Up @@ -395,3 +397,114 @@ describe('runtime-dom: props patching', () => {
expect(fn).toBeCalledTimes(0)
})
})

// #13055
test('should lookup camelCase keys in element properties', async () => {
class TestElement extends HTMLElement {
_complexData = {}

get primitiveValue(): string | null {
return this.getAttribute('primitive-value')
}

set primitiveValue(value: string | undefined) {
if (value) {
this.setAttribute('primitive-value', value)
} else {
this.removeAttribute('primitive-value')
}
}

get complexData() {
return this._complexData
}

set complexData(data) {
this._complexData = data
}
}

window.customElements.define('test-element', TestElement)
const el = document.createElement('test-element') as TestElement

patchProp(el, 'primitive-value', null, 'foo')
expect(el.primitiveValue).toBe('foo')

patchProp(el, 'complex-data', null, { foo: 'bar' })
expect(el.hasAttribute('complex-data')).toBe(false)
expect(el.getAttribute('complex-data')).not.toBe('[object Object]')
expect(el.complexData).toStrictEqual({ foo: 'bar' })
})

// #13055
test('should handle kebab-case prop bindings', async () => {
class HelloWorld extends HTMLElement {
#testProp = ''
#output: HTMLDivElement

get testProp() {
return this.#testProp
}

set testProp(value: string) {
this.#testProp = value
this.#update()
}

constructor() {
super()

this.attachShadow({ mode: 'open' })
this.shadowRoot!.innerHTML = `<div class="output"></div>`
this.#output = this.shadowRoot?.querySelector('.output') as HTMLDivElement
}

connectedCallback() {
console.log('UPDATING!')
this.#update()
}

#update() {
this.#output.innerHTML = `this.testProp = ${this.#testProp}`
}
}

window.customElements.define('hello-world', HelloWorld)

const Comp = defineComponent({
setup() {
const testProp = ref('Hello, world! from App.vue')
return {
testProp,
}
},
template: `
<hello-world .testProp="testProp" />
<hello-world .test-prop="testProp" />
<hello-world .test-prop.camel="testProp" />
`,
})

const root = document.createElement('div')

// Note this one is using the main Vue render so it can compile template
// on the fly
domRender(h(Comp), root)

expect(root.innerHTML).toBe(
`<hello-world></hello-world><hello-world></hello-world><hello-world></hello-world>`,
)

const [child1, child2, child3] = Array.from(
root.querySelectorAll('hello-world'),
)
expect(child3.shadowRoot?.innerHTML).toBe(
'<div class="output">this.testProp = Hello, world! from App.vue</div>',
)
expect(child2.shadowRoot?.innerHTML).toBe(
'<div class="output">this.testProp = Hello, world! from App.vue</div>',
)
expect(child1.shadowRoot?.innerHTML).toBe(
'<div class="output">this.testProp = Hello, world! from App.vue</div>',
)
})
6 changes: 4 additions & 2 deletions packages/runtime-dom/src/patchProp.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ export const patchProp: DOMRendererOptions['patchProp'] = (
? ((key = key.slice(1)), false)
: shouldSetAsProp(el, key, nextValue, isSVG)
) {
patchDOMProp(el, key, nextValue, parentComponent)
const camelKey = camelize(key)
const propKey = camelKey in el ? camelKey : key
patchDOMProp(el, propKey, nextValue, parentComponent, key)
// #6007 also set form state as attributes so they work with
// <input type="reset"> or libs / extensions that expect attributes
// #11163 custom elements may use value as an prop and set it as object
Expand Down Expand Up @@ -140,5 +142,5 @@ function shouldSetAsProp(
return false
}

return key in el
return camelize(key) in el || key in el
Copy link
Member

@edison1105 edison1105 Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe we can cache camelKey on el like this:

const camelKey = camelize(key)
if (camelKey in el) {
  ;(el as any)[`$${key}`] = camelKey
  return true
}

patchProp changes as follows

patchDOMProp(el, el[`$${key}`] || key, nextValue, parentComponent)

Call camelize only once, check in only once

Copy link
Author

Choose a reason for hiding this comment

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

Yes, we can cache the check result in the element itself. I would just like to point that there are 2 situations where we need to perform the check: for .prop bindings and at the end shouldSetAsProp.
For .prop bindings, check would always be performed and only once.
But if we are coming from shouldSetAsProp, to also avoid performing the in check when we don't need to, I would also need to add the $${key} property on the places where shouldSetAsProp would return true - or alternatively set its default value in the top of the function.

I have a question though: is it possible that el would already have a property with the name we are setting ($${key})? Should we actually add a check for that and what to do if it already exists?

function shouldSetAsProp(...) {
  ...
  const camelKey = camelize(key)
  if (camelKey in el) {
    if ((el as any)[`$${key}`] && (el as any)[`$${key}`] !== camelKey) {
      // What to do? Throw?
    }
     ;(el as any)[`$${key}`] = camelKey
    return true
  }
 ...
}

Copy link
Author

Choose a reason for hiding this comment

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

We could use a symbol as key too... either to set directly the value of the current prop being set (would get overwritten by every new attribute being set and not cached across renders) or use it to create an object where properties could then be referenced by their specific key and thus caching the results. Or we leave it as you suggested above. Thoughts?

Copy link
Member

@edison1105 edison1105 Mar 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should we actually add a check for that and what to do if it already exists?

We just need to use a key that is as unlikely to duplicate as possible, such as $_v_prop_${key} or symbol as key.

}