Skip to content

Commit

Permalink
fix: avoid undefined/null expression values (#461)
Browse files Browse the repository at this point in the history
  • Loading branch information
nolanlawson authored Sep 20, 2024
1 parent 3001261 commit 5eb642a
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 3 deletions.
6 changes: 3 additions & 3 deletions src/picker/components/Picker/PickerTemplate.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function render (container, state, helpers, events, actions, refs, abortS
data-ref="rootElement"
class="picker"
aria-label="${state.i18n.regionLabel}"
style="${state.pickerStyle}">
style="${state.pickerStyle || ''}">
<!-- using a spacer div because this allows us to cover up the skintone picker animation -->
<div class="pad-top"></div>
<div class="search-row">
Expand Down Expand Up @@ -81,7 +81,7 @@ export function render (container, state, helpers, events, actions, refs, abortS
aria-expanded="${state.skinTonePickerExpanded}"
aria-controls="skintone-list"
data-on-click="onClickSkinToneButton">
${state.skinToneButtonText}
${state.skinToneButtonText || ''}
</button>
</div>
<span id="skintone-description" class="sr-only">${state.i18n.skinToneDescription}</span>
Expand Down Expand Up @@ -152,7 +152,7 @@ export function render (container, state, helpers, events, actions, refs, abortS
<div class="message ${state.message ? '' : 'gone'}"
role="alert"
aria-live="polite">
${state.message}
${state.message || ''}
</div>
<!--The tabindex=0 is so people can scroll up and down with the keyboard. The element has a role and a label, so I
Expand Down
4 changes: 4 additions & 0 deletions src/picker/components/Picker/framework.js
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@ function patch (expressions, instanceBindings) {

const expression = expressions[expressionIndex]

if (import.meta.env.MODE !== 'production' && (expression === undefined || expression === null)) {
throw new Error('framework does not support undefined or null expressions - these would get stringified as-is')
}

if (currentExpression === expression) {
// no need to update, same as before
continue
Expand Down
27 changes: 27 additions & 0 deletions test/spec/picker/framework.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,33 @@ describe('framework', () => {
expect(node.outerHTML).toBe('<div>baz</div>')
})

test('set expression to undefined/null', () => {
const state = {}
const { html } = createFramework(state)

const renders = [
() => html`<div class="${state.value}"></div>`,
() => html`<div class=${state.value}></div>`,
() => html`<div class="foo ${state.value}"></div>`,
() => html`<div class="${state.value} bar"></div>`,
() => html`<div class="foo ${state.value} bar"></div>`,
() => html`<div>${state.value}</div>`,
() => html`<div>foo ${state.value}</div>`,
() => html`<div>${state.value} bar</div>`,
() => html`<div>foo ${state.value} bar</div>`
]

state.value = undefined
for (const render of renders) {
expect(render).toThrow(/framework does not support undefined or null expressions/)
}

state.value = null
for (const render of renders) {
expect(render).toThrow(/framework does not support undefined or null expressions/)
}
})

// Framework no longer supports this since we switched from HTML comments to text nodes
test.skip('render two dynamic expressions inside the same element', () => {
const state = { name1: 'foo', name2: 'bar' }
Expand Down

0 comments on commit 5eb642a

Please sign in to comment.