Skip to content

Commit

Permalink
feat(compiler): perform automatic key insertion in more situations
Browse files Browse the repository at this point in the history
This expands the scope of the automatic key insertion feature that was
added in #5143. Previously the feature stopped at the border of any
`JsxExpression` syntax node (a bit of arbitrary JavaScript wrapped in
curly braces and then inserted into JSX) so that if you were doing
something like:

```tsx
<div>{ someCondition && <span>when condition is true</span>}</div>
```

the compiler would insert a key into the `<div>` syntax tree node but
_not_ the node for the `<span>`. We did this to just be a bit cautious
with the feature because there are a lot of different things that could
be going on within a `JsxExpression` node, and some things which could
be written within curly braces which would not be safe to transform,
such as the following:

```tsx
<div>{ xs.map(x => <span>{ x }</span>) }</div>
```

We wouldn't want to insert a key into the `<span>` syntax tree node
because that would result in the dynamically generated `<span>` tags
always having the same key attributes.

That said, there are some situations where it is safe to insert a key,
such as the `condition || <some-tag />` case shown above. This change
adds support for inserting keys in those situations.

STENCIL-1120
  • Loading branch information
alicewriteswrongs committed Mar 26, 2024
1 parent ee81472 commit 56814bc
Show file tree
Hide file tree
Showing 2 changed files with 103 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,105 @@ describe('automatic key insertion', () => {
);
});

it('should add a key to a conditionally-rendered static element', async () => {
jest
.spyOn(keyInsertionUtils, 'deriveJSXKey')
.mockReturnValueOnce('my-best-key')
.mockReturnValueOnce('my-worst-key');
const t = transpile(`
@Component({tag: 'cmp-a'})
export class CmpA {
yes = false;
render() {
return (
<div>
{ someConditional && <span>inner</span> }
</div>
)
}
}`);
expect(await formatCode(t.outputText)).toBe(
`export class CmpA {
constructor() {
this.yes = false;
}
render() {
return h('div', { key: 'my-best-key' }, someConditional && h('span', { key: 'my-worst-key' }, 'inner'));
}
static get is() {
return 'cmp-a';
}
}
`,
);
});

it('should not add a key to an IIFE in JSX', async () => {
jest
.spyOn(keyInsertionUtils, 'deriveJSXKey')
.mockReturnValueOnce('my-best-key')
.mockReturnValueOnce('my-worst-key');
const t = transpile(`
@Component({tag: 'cmp-a'})
export class CmpA {
yes = false;
render() {
return (
<div>
{ (() => <div>foo</div>)() }
</div>
)
}
}`);
expect(await formatCode(t.outputText)).toBe(
`export class CmpA {
constructor() {
this.yes = false;
}
render() {
return h('div', { key: 'my-best-key' }, (() => h('div', null, 'foo'))());
}
static get is() {
return 'cmp-a';
}
}
`,
);
});

it('should not add a key within function arguments in JSX', async () => {
jest
.spyOn(keyInsertionUtils, 'deriveJSXKey')
.mockReturnValueOnce('my-best-key')
.mockReturnValueOnce('my-worst-key');
const t = transpile(`
@Component({tag: 'cmp-a'})
export class CmpA {
yes = false;
render() {
return (
<div>
{ func(<div>foo</div>) }
</div>
)
}
}`);
expect(await formatCode(t.outputText)).toBe(
`export class CmpA {
constructor() {
this.yes = false;
}
render() {
return h('div', { key: 'my-worst-key' }, func(h('div', null, 'foo')));
}
static get is() {
return 'cmp-a';
}
}
`,
);
});

it('should not transform JSX in methods with multiple returns', async () => {
jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('shouldnt-see-key');
const t = transpile(`
Expand Down
14 changes: 4 additions & 10 deletions src/compiler/transformers/automatic-key-insertion/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,10 @@ export const performAutomaticKeyInsertion = (transformCtx: ts.TransformationCont
* @returns the result of handling the node
*/
function jsxElementVisitor(node: ts.Node): ts.VisitResult<ts.Node> {
if (ts.isJsxExpression(node)) {
// we don't have the static analysis chops to dive into a JSX expression
// (arbitrary JS code delimited by curly braces in JSX) in order to
// determine whether it's safe to add keys to JSX nodes within it, so we
// bail here instead.
return node;
} else if (ts.isConditionalExpression(node)) {
// we're going to encounter the same problem here that we encounter with
// multiple return statements, so we don't try to transform the arms of
// the conditional.
if (ts.isCallExpression(node) || ts.isPropertyAccessExpression(node) || ts.isConditionalExpression(node)) {
// we're going to encounter the same problems here that we encounter with
// multiple return statements, so we just return the node and don't recur into
// its children
return node;
} else if (isJSXElWithAttrs(node)) {
return addKeyAttr(node);
Expand Down

0 comments on commit 56814bc

Please sign in to comment.