Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 26, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
349df98
test(slot): add karma test for #1968
rwaskiewicz Jul 28, 2023
1df736e
feat(runtime): automatically insert `key` attrs during compilation
alicewriteswrongs Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
315 changes: 315 additions & 0 deletions src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
}
}
`,
);
});
Comment on lines +207 to +237
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a test to ensure that we don't add static keys to dynamic generated elements. I did a bit of a conservative cut-off for these - we don't attempt to transform any JSX nodes which are found within a JSX expression, so with (contrived) code like

@Component({ tag: 'cmp-a' })
class CmpA {
  render() {
    return <div>{ (() => <div>inner</div>)() }</div>
  }
}

we won't transform the 'inner' div in the IIFE because, since it will be generated at runtime, we could run into a lot of cases where we can't really know too much about what's going on there and when it would not be safe to add keys.


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');
}
}
`,
);
});
});
Loading
Loading