Skip to content

Commit

Permalink
Exclude HTML elements from template locals by default
Browse files Browse the repository at this point in the history
Currently, all valid identifiers will be included in the output,
including HTML like identifiers. This can cause issues that are not
obvious from the get go, for instance:

```js
test('Supports the on modifier', async (assert) => {
  class MyComponent extends Component {
    static template = hbs`<button {{on "click" this.incrementCounter}}>Count: {{this.count}}</button>`;
    @Tracked count = 0;

    @action
    incrementCounter() {
      this.count++;
    }
  }

  const element = document.getElementById('qunit-fixture')!;

  await renderComponent(MyComponent, element);
  assert.strictEqual(
    element.innerHTML,
    `<button>Count: 0</button>`,
    'the component was rendered'
  );

  const button = element.querySelector('button')!;
  button.click();

  await didRender();
  assert.strictEqual(
    element.innerHTML,
    `<button>Count: 1</button>`,
    'the component was rerendered'
  );
});
```

This test fails because `button` is technically an in-scope variable,
declared _after_ `renderComponent` is called. Using the template
transform, it thinks that `button` really exists and is a component
definition, and throws a ReferenceError when it attempts to access it
because it hasn't been defined yet.

This change would allow us to ignore any identifier that is:

1. Only used in angle bracket invocation
2. Has no path segments
3. Is all lower case

Preventing these collisions from happening with common HTML elements
such as `button`, `span`, `div`, etc. This also follows JSX's rules for
determining if a [value in scope is a component](https://reactjs.org/docs/jsx-in-depth.html#html-tags-vs.-react-components),
so at the least this strategy is already used in a real world framework
without much confusion.
  • Loading branch information
Chris Garrett committed Mar 23, 2021
1 parent 396dd10 commit 2c964d7
Show file tree
Hide file tree
Showing 2 changed files with 61 additions and 6 deletions.
36 changes: 30 additions & 6 deletions packages/@glimmer/syntax/lib/get-template-locals.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,19 @@ import { preprocess } from './parser/tokenizer-event-handlers';
import traverse from './traversal/traverse';
import * as ASTv1 from './v1/api';

interface GetTemplateLocalsOptions {
includeKeywords?: boolean;
includeHtmlElements?: boolean;
}

/**
* Gets the correct Token from the Node based on it's type
*/
function tokensFromType(node: ASTv1.Node, scopedTokens: string[]): string | void {
function tokensFromType(
node: ASTv1.Node,
scopedTokens: string[],
options: GetTemplateLocalsOptions
): string | void {
if (node.type === 'PathExpression') {
if (node.head.type === 'AtHead' || node.head.type === 'ThisHead') {
return;
Expand All @@ -26,6 +35,10 @@ function tokensFromType(node: ASTv1.Node, scopedTokens: string[]): string | void
return;
}

if (!options.includeHtmlElements && tag.indexOf('.') === -1 && tag.toLowerCase() === tag) {
return;
}

if (tag.substr(0, 5) === 'this.') {
return;
}
Expand All @@ -41,8 +54,13 @@ function tokensFromType(node: ASTv1.Node, scopedTokens: string[]): string | void
/**
* Adds tokens to the tokensSet based on their node.type
*/
function addTokens(tokensSet: Set<string>, node: ASTv1.Node, scopedTokens: string[]) {
const maybeTokens = tokensFromType(node, scopedTokens);
function addTokens(
tokensSet: Set<string>,
node: ASTv1.Node,
scopedTokens: string[],
options: GetTemplateLocalsOptions
) {
const maybeTokens = tokensFromType(node, scopedTokens, options);

(Array.isArray(maybeTokens) ? maybeTokens : [maybeTokens]).forEach((maybeToken) => {
if (maybeToken !== undefined && maybeToken[0] !== '@') {
Expand All @@ -56,7 +74,13 @@ function addTokens(tokensSet: Set<string>, node: ASTv1.Node, scopedTokens: strin
* referenced that could possible come from the praent scope. Can exclude known keywords
* optionally.
*/
export function getTemplateLocals(html: string, options?: { includeKeywords: boolean }): string[] {
export function getTemplateLocals(
html: string,
options: GetTemplateLocalsOptions = {
includeHtmlElements: false,
includeKeywords: false,
}
): string[] {
const ast = preprocess(html);
const tokensSet = new Set<string>();
const scopedTokens: string[] = [];
Expand All @@ -81,7 +105,7 @@ export function getTemplateLocals(html: string, options?: { includeKeywords: boo
node.blockParams.forEach((param) => {
scopedTokens.push(param);
});
addTokens(tokensSet, node, scopedTokens);
addTokens(tokensSet, node, scopedTokens, options);
},

exit({ blockParams }) {
Expand All @@ -92,7 +116,7 @@ export function getTemplateLocals(html: string, options?: { includeKeywords: boo
},

PathExpression(node) {
addTokens(tokensSet, node, scopedTokens);
addTokens(tokensSet, node, scopedTokens, options);
},
});

Expand Down
31 changes: 31 additions & 0 deletions packages/@glimmer/syntax/test/template-locals-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ QUnit.test('it works', function (assert) {
{{#this.dynamicBlockComponent}}
{{/this.dynamicBlockComponent}}
<button></button>
<this.dynamicAngleComponent>
</this.dynamicAngleComponent>
`);
Expand All @@ -46,6 +48,22 @@ QUnit.test('it works', function (assert) {
]);
});

QUnit.test('it does not include locals', function (assert) {
let locals = getTemplateLocals(
`
<SomeComponent as |button|>
<button></button>
{{button}}
</SomeComponent>
`,
{
includeHtmlElements: true,
}
);

assert.deepEqual(locals, ['SomeComponent']);
});

QUnit.test('it can include keywords', function (assert) {
let locals = getTemplateLocals(
`
Expand Down Expand Up @@ -96,3 +114,16 @@ QUnit.test('it can include keywords', function (assert) {
'someOther',
]);
});

QUnit.test('it can include html elements', function (assert) {
let locals = getTemplateLocals(
`
<button></button>
`,
{
includeHtmlElements: true,
}
);

assert.deepEqual(locals, ['button']);
});

0 comments on commit 2c964d7

Please sign in to comment.