Skip to content

Commit

Permalink
Don't allow use of the nullish coalescing operator in templates (#2025)
Browse files Browse the repository at this point in the history
# Pull Request

## 🤨 Rationale

Resolves #1843 

I did an evaluation of how observables work within FAST templates. A
longer explanation of what is and is not allowed can be found [in the
discussion within the linked
issue](#1843 (comment)).
The only issue I found within nimble code was the usage of `??` in
templates, which may not correctly be identified as a volatile binding.
Therefore, I've rewritten the bindings that previously used `??` and
added a lint rule for `template.ts` files that disallows usage of `??`.

## 👩‍💻 Implementation

See Rationale

## 🧪 Testing

All existing tests still pass
Verified that the new lint rule catches instances of `??` in
`template.ts` files

## ✅ Checklist

<!--- Review the list and put an x in the boxes that apply or ~~strike
through~~ around items that don't (along with an explanation). -->

- [ ] I have updated the project documentation to reflect my changes or
determined no changes are needed.
  • Loading branch information
mollykreis authored Apr 19, 2024
1 parent 89ce7be commit e3e5231
Show file tree
Hide file tree
Showing 5 changed files with 23 additions and 5 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
{
"type": "patch",
"comment": "Don't allow use of the nullish coalescing operator in templates",
"packageName": "@ni/nimble-components",
"email": "[email protected]",
"dependentChangeType": "patch"
}
11 changes: 11 additions & 0 deletions packages/eslint-config-nimble/typescript.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,17 @@ module.exports = {
'@typescript-eslint/indent': 'off'
}
},
{
files: ['template.ts'],
rules: {
// Using '??' in templates does not get flagged correctly by FAST as being a volatile binding.
// See https://github.com/ni/nimble/issues/1843 for more information.
'no-restricted-syntax': [
'error',
{ selector: "LogicalExpression[operator='??']" }
]
}
},
{
// Instead of enums, this repo uses const objects and type unions which should live in types.ts
files: ['types.ts'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ export const template = html<RichTextEditor>`
${ref('mentionListbox')}
@mention-selected=${(x, c) => x.onMentionSelect(c.event as CustomEvent<MentionDetail>)}
>
${repeat(x => Array.from(x.activeMappingConfigs?.values() ?? []), html<MappingConfig>`
${repeat(x => Array.from(x.activeMappingConfigs ? x.activeMappingConfigs.values() : []), html<MappingConfig>`
<${listOptionTag} value="${x => x.mentionHref}">${x => x.displayName}</${listOptionTag}>
`, { recycle: false })}
</${richTextMentionListboxTag}>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ export const template = html<TableCell>`
@toggle="${(x, c) => x.onActionMenuToggle(c.event as CustomEvent<MenuButtonToggleEventDetail>)}"
@click="${(_, c) => c.event.stopPropagation()}"
class="action-menu"
title="${x => x.actionMenuLabel ?? tableCellActionMenuLabel.getValueFor(x)}"
title="${x => (x.actionMenuLabel ? x.actionMenuLabel : tableCellActionMenuLabel.getValueFor(x))}"
>
<${iconThreeDotsLineTag} slot="start"></${iconThreeDotsLineTag}>
${x => x.actionMenuLabel ?? tableCellActionMenuLabel.getValueFor(x)}
${x => (x.actionMenuLabel ? x.actionMenuLabel : tableCellActionMenuLabel.getValueFor(x))}
<slot name="cellActionMenu" slot="menu"></slot>
</${menuButtonTag}>
`)}
Expand Down
4 changes: 2 additions & 2 deletions packages/nimble-components/src/table/template.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const template = html<Table>`
--ni-private-table-header-container-margin-right: ${x => x.virtualizer.headerContainerMarginRight}px;
--ni-private-table-scroll-height: ${x => x.virtualizer.scrollHeight}px;
--ni-private-table-row-container-top: ${x => x.virtualizer.rowContainerYOffset}px;
--ni-private-table-row-grid-columns: ${x => x.rowGridColumns ?? ''};
--ni-private-table-row-grid-columns: ${x => (x.rowGridColumns ? x.rowGridColumns : '')};
--ni-private-table-cursor-override: ${x => (x.layoutManager.isColumnBeingSized ? 'col-resize' : 'default')};
--ni-private-table-scrollable-min-width: ${x => x.tableScrollableMinWidth}px;
--ni-private-glass-overlay-pointer-events: ${x => (x.layoutManager.isColumnBeingSized ? 'none' : 'default')};
Expand All @@ -60,7 +60,7 @@ export const template = html<Table>`
<span class="checkbox-container">
<${checkboxTag}
${ref('selectionCheckbox')}
class="${x => `selection-checkbox ${x.selectionMode ?? ''}`}"
class="${x => `selection-checkbox ${x.selectionMode ? x.selectionMode : ''}`}"
@change="${(x, c) => x.onAllRowsSelectionChange(c.event as CustomEvent)}"
title="${x => tableSelectAllLabel.getValueFor(x)}"
aria-label="${x => tableSelectAllLabel.getValueFor(x)}"
Expand Down

0 comments on commit e3e5231

Please sign in to comment.