feat(runtime): automatically insert key attrs during compilation (#…

…5143)

* test(slot): add karma test for #1968

this commit adds a karma test for #1968, based on the minimal
reproduction case that we received for said issue

* feat(runtime): automatically insert `key` attrs during compilation

This adds a new transformer, `performAutomaticKeyInsertion`, which will
add `key` attributes into certain JSX nodes in the `render()` method of
a Stencil component. This will allow the Stencil runtime to make more
accurate decisions about when to create and destroy child nodes during
re-rendering, especially in circumstances where nodes are changing order
and so on.

There are some limitations on when we can safely insert keys without
possibly introducing incorrect behavior. In particular, we don't want to
insert keys in the following situations:

- when a `render` method has more than one return statement
- when a `render` method has a conditional (ternary) expression in it
- when a JSX node is located inside of a JSX expression (i.e. within
  curly braces like `<div>{ ... }</div>`)

In each of these cases we don't have the static analysis chops to know
when a given JSX syntax tree node is supposed to correspond to the same
HTML element at runtime, whereas in the 'single return, JSX children
case' like:

```tsx
class Component {
  render () {
    return <div><img /></div>
  }
}
```

We know without a doubt in this case that the `div` and `img` JSX nodes
are always supposed to correspond to the same HTML elements at runtime,
so it's no problem to add keys to them.

The key insertion transformer also does not transform JSX nodes which
are not part of Stencil components, so if a project had, for some
reason, a React component or something in it we will leave it alone.

fixes #1968
fixes #5263
STENCIL-893

---------

Co-authored-by: Ryan Waskiewicz <[email protected]>

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

@@ -0,0 +1,315 @@
+import { transpileModule } from '../test/transpile';
+import { formatCode } from '../test/utils';
+import * as keyInsertionUtils from './utils';
+
+function transpile(code: string) {
+  return transpileModule(code, null, null, []);
+}
+
+describe('automatic key insertion', () => {
+  afterEach(() => {
+    jest.clearAllMocks();
+  });
+
+  it('should add a key to one JSX opener', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('test-key');
+    const t = transpile(`
+    @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <div>test</div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', { key: 'test-key' }, 'test');
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should add a key to nested JSX', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('key1').mockReturnValueOnce('key2');
+    const t = transpile(`
+    @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <div>test<img src="image.png" /></div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', { key: 'key1' }, 'test', h('img', { key: 'key2', src: 'image.png' }));
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should add a key to one JSX opener w/ existing attr', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('test-key');
+    const t = transpile(`
+    @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <div class="foo">test</div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', { key: 'test-key', class: 'foo' }, 'test');
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should add a key to a self-closing JSX element', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('img-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <img />
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('img', { key: 'img-key' });
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should add a key to a self-closing JSX element w/ existing attr', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('img-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <img src="my-img.png" />
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('img', { key: 'img-key', src: 'my-img.png' });
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should add unique keys to multiple JSX elements', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('first-key').mockReturnValueOnce('second-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <div><img src="my-img.png" /></div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', { key: 'first-key' }, h('img', { key: 'second-key', src: 'my-img.png' }));
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should respect an existing key', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('never-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+      export class CmpA {
+        render() {
+          return <div key="my-key">hey</div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', { key: 'my-key' }, 'hey');
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should respect an existing key in a loop', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('once-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+     export class CmpA {
+        render() {
+          return (
+            <div>
+              {this.todos.map((todo) => (
+                <div key={todo}>{ todo }</div>
+              ))}
+            </div>
+          )
+      }
+    }`);
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h(
+      'div',
+      { key: 'once-key' },
+      this.todos.map((todo) => h('div', { key: todo }, todo)),
+    );
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should not add a static key to dynamic elements', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValueOnce('once-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+     export class CmpA {
+        render() {
+          return (
+            <div>
+              {this.todos.map((todo) => (
+                <div>{ todo }</div>
+              ))}
+            </div>
+          )
+      }
+    }`);
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h(
+      'div',
+      { key: 'once-key' },
+      this.todos.map((todo) => h('div', null, todo)),
+    );
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should not transform JSX inside of a ternary', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue('shouldnt-see-key');
+    const t = transpile(`
+     @Component({tag: 'cmp-a'})
+     export class CmpA {
+       yes = false;
+       render() {
+         return this.yes ? <span>yes</span> : <span>no</span>
+       }
+     }`);
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  constructor() {
+    this.yes = false;
+  }
+  render() {
+    return this.yes ? h('span', null, 'yes') : h('span', null, 'no');
+  }
+  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(`
+     @Component({tag: 'cmp-a'})
+     export class CmpA {
+       booleo = false;
+       render() {
+         if (this.booleo) {
+           return <div>early!</div>;
+         }
+         return <div>late!</div>;
+       }
+     }`);
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  constructor() {
+    this.booleo = false;
+  }
+  render() {
+    if (this.booleo) {
+      return h('div', null, 'early!');
+    }
+    return h('div', null, 'late!');
+  }
+  static get is() {
+    return 'cmp-a';
+  }
+}
+`,
+    );
+  });
+
+  it('should not edit a non-stencil class', async () => {
+    jest.spyOn(keyInsertionUtils, 'deriveJSXKey').mockReturnValue("shouldn't see this!");
+    const t = transpile(`
+      export class CmpA {
+        render() {
+          return <div>hey</div>
+      }
+    }`);
+
+    expect(await formatCode(t.outputText)).toBe(
+      `export class CmpA {
+  render() {
+    return h('div', null, 'hey');
+  }
+}
+`,
+    );
+  });
+});