feat(compiler): perform automatic key insertion in more situations (#…

…5594)

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

src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts

@@ -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(`

src/compiler/transformers/automatic-key-insertion/index.ts

@@ -71,11 +71,33 @@ export const performAutomaticKeyInsertion = (transformCtx: ts.TransformationCont
    * @returns the result of handling the node
    */
   function findRenderMethodVisitor(node: ts.Node): ts.VisitResult<ts.Node> {
-    // we want to keep going (to drill down into JSX nodes and transform them) only in particular circumstances:
+    // we want to keep going (to drill down into JSX nodes and transform them)
+    // only in particular circumstances:
     //
     // 1. the syntax tree node is a method declaration
     // 2. this method's name is 'render'
     // 3. the method only has a single return statement
+    //
+    // We want to only keep going if there's a single return statement because
+    // if there are multiple return statements inserting keys could cause
+    // needless re-renders. If a `render` method looked like this, for
+    // instance:
+    //
+    // ```tsx
+    // render() {
+    //   if (foo) {
+    //     return <div>hey!</div>;
+    //   } else {
+    //     return <div>hay!</div>;
+    //   }
+    // }
+    // ```
+    //
+    // Since the `<div>` tags don't have `key` attributes the Stencil vdom will
+    // re-use the same div element between re-renders, and will just swap out
+    // the children (the text nodes in this case). If our key insertion
+    // transformer put unique keys onto each tag then this wouldn't happen any
+    // longer.
     if (ts.isMethodDeclaration(node) && node.name.getText() === 'render' && numReturnStatements(node) === 1) {
       return ts.visitEachChild(node, jsxElementVisitor, transformCtx);
     } else {
@@ -91,16 +113,15 @@ 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.
+    if (ts.isCallExpression(node)) {
+      // if there are any JSX nodes which are children of the call expression
+      // (i.e. arguments) we don't want to transform them since we can't know
+      // _a priori_ what could be done with them at runtime
       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.
+      // 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);