From 9c47438a9a727c9d21cc7441e022097a966bd60d Mon Sep 17 00:00:00 2001 From: Alice Pote Date: Fri, 26 Jan 2024 14:52:08 -0500 Subject: [PATCH] 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 `
{ ... }
`) 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
} } ``` 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 --- .../automatic-key-insertion.spec.ts | 315 ++++++++++++++++++ .../automatic-key-insertion/index.ts | 232 +++++++++++++ .../automatic-key-insertion/utils.ts | 36 ++ src/compiler/transformers/test/transpile.ts | 7 +- src/compiler/transpile/run-program.ts | 6 +- src/compiler/transpile/transpile-module.ts | 2 + src/runtime/test/render-vdom.spec.tsx | 34 +- test/karma/test-app/components.d.ts | 15 + .../karma/test-app/scoped-conditional/cmp.tsx | 31 ++ .../test-app/scoped-conditional/index.html | 16 + .../test-app/scoped-conditional/karma.spec.ts | 61 ++++ test/karma/test-app/svg-attr/karma.spec.ts | 4 +- 12 files changed, 740 insertions(+), 19 deletions(-) create mode 100644 src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts create mode 100644 src/compiler/transformers/automatic-key-insertion/index.ts create mode 100644 src/compiler/transformers/automatic-key-insertion/utils.ts create mode 100644 test/karma/test-app/scoped-conditional/cmp.tsx create mode 100644 test/karma/test-app/scoped-conditional/index.html create mode 100644 test/karma/test-app/scoped-conditional/karma.spec.ts diff --git a/src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts b/src/compiler/transformers/automatic-key-insertion/automatic-key-insertion.spec.ts new file mode 100644 index 00000000000..01384222f78 --- /dev/null +++ b/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
test
+ } + }`); + + 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
test
+ } + }`); + + 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
test
+ } + }`); + + 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 + } + }`); + + 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 + } + }`); + + 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
+ } + }`); + + 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
hey
+ } + }`); + + 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 ( +
+ {this.todos.map((todo) => ( +
{ todo }
+ ))} +
+ ) + } + }`); + 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 ( +
+ {this.todos.map((todo) => ( +
{ todo }
+ ))} +
+ ) + } + }`); + 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 ? yes : no + } + }`); + 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
early!
; + } + return
late!
; + } + }`); + 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
hey
+ } + }`); + + expect(await formatCode(t.outputText)).toBe( + `export class CmpA { + render() { + return h('div', null, 'hey'); + } +} +`, + ); + }); +}); diff --git a/src/compiler/transformers/automatic-key-insertion/index.ts b/src/compiler/transformers/automatic-key-insertion/index.ts new file mode 100644 index 00000000000..8fab80f0505 --- /dev/null +++ b/src/compiler/transformers/automatic-key-insertion/index.ts @@ -0,0 +1,232 @@ +import ts from 'typescript'; + +import { getComponentTagName, isStaticGetter } from '../transform-utils'; +import { deriveJSXKey } from './utils'; + +/** + * A transformer factory to create a transformer which will add `key` + * properties to all of the JSX nodes contained inside of a Stencil component's + * `render` function. + * + * This can be thought of as transforming the following: + * + * ```tsx + * class MyComponent { + * render() { + *
hey!
+ * } + * } + * ``` + * + * to this: + * + * ```tsx + * class MyComponent { + * render() { + *
hey!
+ * } + * } + * ``` + * + * The inserted keys are generated by {@link deriveJSXKey}. + * + * **Note**: this transformer must be run _after_ the + * `convertDecoratorsToStatic` transformer, since it depends on static getters + * created by that transformer to determine when to transform a class node. + * + * @param transformCtx a transformation context + * @returns a typescript transformer for inserting keys into JSX nodes + */ +export const performAutomaticKeyInsertion = (transformCtx: ts.TransformationContext): ts.Transformer => { + /** + * This is our outer-most visitor function which serves to locate a class + * declaration which is also a Stencil component, at which point it hands + * things over to the next visitor function ({@link findRenderMethodVisitor}) + * which locates the `render` method. + * + * @param node a typescript syntax tree node + * @returns the result of handling the node + */ + function findClassDeclVisitor(node: ts.Node): ts.VisitResult { + if (ts.isClassDeclaration(node)) { + const tagName = getComponentTagName(node.members.filter(isStaticGetter)); + if (tagName != null) { + // we've got a class node with an `is` property, which tells us that + // the class we're dealing with is a Stencil component which has + // already been through the `convertDecoratorsToStatic` transformer. + return ts.visitEachChild(node, findRenderMethodVisitor, transformCtx); + } + } + // we either didn't find a class node, or we found a class node without a + // component tag name, so this is not a stencil component! + return ts.visitEachChild(node, findClassDeclVisitor, transformCtx); + } + + /** + * This middle visitor function is responsible for finding the render method + * on a Stencil class and then passing off responsibility to the inner-most + * visitor, which deals with syntax nodes inside the method. + * + * @param node a typescript syntax tree node + * @returns the result of handling the node + */ + function findRenderMethodVisitor(node: ts.Node): ts.VisitResult { + // 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 + if (ts.isMethodDeclaration(node) && node.name.getText() === 'render' && numReturnStatements(node) === 1) { + return ts.visitEachChild(node, jsxElementVisitor, transformCtx); + } else { + return ts.visitEachChild(node, findRenderMethodVisitor, transformCtx); + } + } + + /** + * Our inner-most visitor function. This will edit any JSX nodes that it + * finds, adding a `key` attribute to them via {@link addKeyAttr}. + * + * @param node a typescript syntax tree node + * @returns the result of handling the node + */ + function jsxElementVisitor(node: ts.Node): ts.VisitResult { + 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. + return node; + } else if (isJSXElWithAttrs(node)) { + return addKeyAttr(node); + } else { + return ts.visitEachChild(node, jsxElementVisitor, transformCtx); + } + } + + return (tsSourceFile) => { + return ts.visitEachChild(tsSourceFile, findClassDeclVisitor, transformCtx); + }; +}; + +/** + * Count the number of return statements in a {@link ts.MethodDeclaration} + * + * @param method the node within which we're going to count `return` statements + * @returns the number of return statements found + */ +function numReturnStatements(method: ts.MethodDeclaration): number { + let count = 0; + + function walker(node: ts.Node) { + for (const child of node.getChildren()) { + if (ts.isReturnStatement(child)) { + count++; + } else { + walker(child); + } + } + } + + walker(method); + + return count; +} + +/** + * Type guard to see if a TypeScript syntax node is one of the node types which + * corresponds to a JSX element that can have attributes on it. This is either + * an opening node, like `
`, or a 'self-closing' node like + * ``. + * + * @param node a typescript syntax tree node + * @returns whether or not the node is JSX node which could have attributes + */ +function isJSXElWithAttrs(node: ts.Node): node is ts.JsxOpeningElement | ts.JsxSelfClosingElement { + return ts.isJsxOpeningElement(node) || ts.isJsxSelfClosingElement(node); +} + +/** + * Given a JSX syntax tree node update it to include a unique key attribute. + * This will respect any attributes already set on the node, including a + * pre-existing, user-defined `key` attribute. + * + * @param jsxElement a typescript JSX syntax tree node + * @returns an updated JSX element, with a key added. + */ +function addKeyAttr( + jsxElement: ts.JsxOpeningElement | ts.JsxSelfClosingElement, +): ts.JsxOpeningElement | ts.JsxSelfClosingElement { + if (jsxElement.attributes.properties.some(isKeyAttr)) { + // this node already has a key! let's get out of here + return jsxElement; + } + + const updatedAttributes = ts.factory.createJsxAttributes([ + ts.factory.createJsxAttribute( + ts.factory.createIdentifier('key'), + ts.factory.createStringLiteral(deriveJSXKey(jsxElement)), + ), + ...jsxElement.attributes.properties, + ]); + + if (ts.isJsxOpeningElement(jsxElement)) { + return ts.factory.updateJsxOpeningElement( + jsxElement, + jsxElement.tagName, + jsxElement.typeArguments, + updatedAttributes, + ); + } else { + return ts.factory.updateJsxSelfClosingElement( + jsxElement, + jsxElement.tagName, + jsxElement.typeArguments, + updatedAttributes, + ); + } +} + +/** + * Check whether or not a JSX attribute node (well, technically a + * {@link ts.JsxAttributeLike} node) has the name `"key"` or not + * + * @param attr the JSX attribute node to check + * @returns whether or not this node has the name 'key' + */ +function isKeyAttr(attr: ts.JsxAttributeLike): boolean { + return !!attr.name && attrNameToString(attr) === 'key'; +} + +/** + * Given a JSX attribute get its name as a string + * + * @param attr the attribute of interest + * @returns the attribute's name, formatted as a string + */ +function attrNameToString(attr: ts.JsxAttributeLike): string { + switch (attr.name?.kind) { + case ts.SyntaxKind.Identifier: + case ts.SyntaxKind.PrivateIdentifier: + case ts.SyntaxKind.StringLiteral: + case ts.SyntaxKind.NumericLiteral: + return attr.name.text; + case ts.SyntaxKind.JsxNamespacedName: + // this is a JSX attribute name like `foo:bar` + // see https://facebook.github.io/jsx/#prod-JSXNamespacedName + return attr.name.getText(); + case ts.SyntaxKind.ComputedPropertyName: + const expression = attr.name.expression; + if (ts.isStringLiteral(expression) || ts.isNumericLiteral(expression)) { + return expression.text; + } + return ''; + default: + return ''; + } +} diff --git a/src/compiler/transformers/automatic-key-insertion/utils.ts b/src/compiler/transformers/automatic-key-insertion/utils.ts new file mode 100644 index 00000000000..8d854dc77aa --- /dev/null +++ b/src/compiler/transformers/automatic-key-insertion/utils.ts @@ -0,0 +1,36 @@ +import { createHash } from 'node:crypto'; + +import ts from 'typescript'; + +/** + * An incrementing-number generator, just as a little extra 'uniqueness' + * insurance for {@link deriveJSXKey} + */ +const incrementer = (function* () { + let val = 0; + while (true) { + yield val++; + } +})(); + +/** + * Generate a unique key for a given JSX element. The key is creating by + * concatenating and then hashing (w/ SHA1) the following: + * + * - an incrementing value + * - the element's tag name + * - the start position for the element's token in the original source file + * - the end position for the element's token in the original source file + * + * It is hoped this provides enough uniqueness that a collision won't occur. + * + * @param jsxElement a typescript JSX syntax tree node which needs a key + * @returns a computed unique key for that element + */ +export function deriveJSXKey(jsxElement: ts.JsxOpeningElement | ts.JsxSelfClosingElement): string { + const hash = createHash('sha1') + .update(`${incrementer.next().value}__${jsxElement.tagName}__${jsxElement.pos}_${jsxElement.end}`) + .digest('hex') + .toLowerCase(); + return hash; +} diff --git a/src/compiler/transformers/test/transpile.ts b/src/compiler/transformers/test/transpile.ts index 97adedaad36..48bd4b7419d 100644 --- a/src/compiler/transformers/test/transpile.ts +++ b/src/compiler/transformers/test/transpile.ts @@ -2,6 +2,7 @@ import type * as d from '@stencil/core/declarations'; import { mockBuildCtx, mockCompilerCtx, mockValidatedConfig } from '@stencil/core/testing'; import ts from 'typescript'; +import { performAutomaticKeyInsertion } from '../automatic-key-insertion'; import { convertDecoratorsToStatic } from '../decorators-to-static/convert-decorators'; import { updateModule } from '../static-to-meta/parse-static'; import { convertStaticToMeta } from '../static-to-meta/visitor'; @@ -109,7 +110,11 @@ export function transpileModule( }; tsProgram.emit(undefined, undefined, undefined, undefined, { - before: [convertDecoratorsToStatic(config, buildCtx.diagnostics, tsTypeChecker, tsProgram), ...beforeTransformers], + before: [ + convertDecoratorsToStatic(config, buildCtx.diagnostics, tsTypeChecker, tsProgram), + performAutomaticKeyInsertion, + ...beforeTransformers, + ], after: [ convertStaticToMeta(config, compilerCtx, buildCtx, tsTypeChecker, null, transformOpts), ...afterTransformers, diff --git a/src/compiler/transpile/run-program.ts b/src/compiler/transpile/run-program.ts index 70b4da1d8fc..129650a565d 100644 --- a/src/compiler/transpile/run-program.ts +++ b/src/compiler/transpile/run-program.ts @@ -12,6 +12,7 @@ import ts from 'typescript'; import type * as d from '../../declarations'; import { updateComponentBuildConditionals } from '../app-core/app-data'; import { resolveComponentDependencies } from '../entries/resolve-component-dependencies'; +import { performAutomaticKeyInsertion } from '../transformers/automatic-key-insertion'; import { convertDecoratorsToStatic } from '../transformers/decorators-to-static/convert-decorators'; import { rewriteAliasedDTSImportPaths } from '../transformers/rewrite-aliased-paths'; import { updateModule } from '../transformers/static-to-meta/parse-static'; @@ -66,7 +67,10 @@ export const runTsProgram = async ( }; const transformers: ts.CustomTransformers = { - before: [convertDecoratorsToStatic(config, buildCtx.diagnostics, tsTypeChecker, tsProgram)], + before: [ + convertDecoratorsToStatic(config, buildCtx.diagnostics, tsTypeChecker, tsProgram), + performAutomaticKeyInsertion, + ], afterDeclarations: [], }; diff --git a/src/compiler/transpile/transpile-module.ts b/src/compiler/transpile/transpile-module.ts index b5fb480e108..26d173cda50 100644 --- a/src/compiler/transpile/transpile-module.ts +++ b/src/compiler/transpile/transpile-module.ts @@ -5,6 +5,7 @@ import ts from 'typescript'; import type * as d from '../../declarations'; import { BuildContext } from '../build/build-ctx'; import { CompilerContext } from '../build/compiler-ctx'; +import { performAutomaticKeyInsertion } from '../transformers/automatic-key-insertion'; import { lazyComponentTransform } from '../transformers/component-lazy/transform-lazy-component'; import { nativeComponentTransform } from '../transformers/component-native/tranform-to-native-component'; import { convertDecoratorsToStatic } from '../transformers/decorators-to-static/convert-decorators'; @@ -113,6 +114,7 @@ export const transpileModule = ( const transformers: ts.CustomTransformers = { before: [ convertDecoratorsToStatic(config, buildCtx.diagnostics, typeChecker, program), + performAutomaticKeyInsertion, updateStencilCoreImports(transformOpts.coreImportPath), ], after: [convertStaticToMeta(config, compilerCtx, buildCtx, typeChecker, null, transformOpts)], diff --git a/src/runtime/test/render-vdom.spec.tsx b/src/runtime/test/render-vdom.spec.tsx index c340d0b0a85..8a6c1f68bac 100644 --- a/src/runtime/test/render-vdom.spec.tsx +++ b/src/runtime/test/render-vdom.spec.tsx @@ -15,11 +15,11 @@ describe('render-vdom', () => { const { build } = await newSpecPage({ components: [CmpA], strictBuild: true }); expect(build).toMatchObject({ - vdomAttribute: false, + vdomAttribute: true, vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -38,11 +38,11 @@ describe('render-vdom', () => { const { build } = await newSpecPage({ components: [CmpA], strictBuild: true }); expect(build).toMatchObject({ - vdomAttribute: false, + vdomAttribute: true, vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -61,11 +61,11 @@ describe('render-vdom', () => { const { build } = await newSpecPage({ components: [CmpA], strictBuild: true }); expect(build).toMatchObject({ - vdomAttribute: false, + vdomAttribute: true, vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -84,11 +84,11 @@ describe('render-vdom', () => { const { build } = await newSpecPage({ components: [CmpA], strictBuild: true }); expect(build).toMatchObject({ - vdomAttribute: false, + vdomAttribute: true, vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -110,7 +110,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: true, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -132,7 +132,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: false, vdomStyle: true, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -182,7 +182,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: true, vdomListener: false, vdomFunctional: false, @@ -210,7 +210,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: true, vdomFunctional: false, @@ -238,7 +238,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: true, vdomFunctional: false, @@ -337,7 +337,7 @@ describe('render-vdom', () => { vdomXlink: false, vdomClass: false, vdomStyle: false, - vdomKey: false, + vdomKey: true, vdomRef: false, vdomListener: false, vdomFunctional: false, @@ -1073,8 +1073,10 @@ describe('render-vdom', () => { @Component({ tag: 'cmp-a' }) class CmpA { counter = 0; - setRef = () => { - this.counter++; + setRef = (el: HTMLDivElement | null) => { + if (el !== null) { + this.counter++; + } }; @Prop() state = true; diff --git a/test/karma/test-app/components.d.ts b/test/karma/test-app/components.d.ts index 32369fd22f1..6d3a02a9e6a 100644 --- a/test/karma/test-app/components.d.ts +++ b/test/karma/test-app/components.d.ts @@ -252,6 +252,9 @@ export namespace Components { } interface ScopedBasicRoot { } + interface ScopedConditional { + "renderHello": boolean; + } interface ScopedSlotAppendAndPrepend { } interface ScopedSlotChildInsertAdjacent { @@ -1076,6 +1079,12 @@ declare global { prototype: HTMLScopedBasicRootElement; new (): HTMLScopedBasicRootElement; }; + interface HTMLScopedConditionalElement extends Components.ScopedConditional, HTMLStencilElement { + } + var HTMLScopedConditionalElement: { + prototype: HTMLScopedConditionalElement; + new (): HTMLScopedConditionalElement; + }; interface HTMLScopedSlotAppendAndPrependElement extends Components.ScopedSlotAppendAndPrepend, HTMLStencilElement { } var HTMLScopedSlotAppendAndPrependElement: { @@ -1547,6 +1556,7 @@ declare global { "sass-cmp": HTMLSassCmpElement; "scoped-basic": HTMLScopedBasicElement; "scoped-basic-root": HTMLScopedBasicRootElement; + "scoped-conditional": HTMLScopedConditionalElement; "scoped-slot-append-and-prepend": HTMLScopedSlotAppendAndPrependElement; "scoped-slot-child-insert-adjacent": HTMLScopedSlotChildInsertAdjacentElement; "shadow-dom-array": HTMLShadowDomArrayElement; @@ -1861,6 +1871,9 @@ declare namespace LocalJSX { } interface ScopedBasicRoot { } + interface ScopedConditional { + "renderHello"?: boolean; + } interface ScopedSlotAppendAndPrepend { } interface ScopedSlotChildInsertAdjacent { @@ -2100,6 +2113,7 @@ declare namespace LocalJSX { "sass-cmp": SassCmp; "scoped-basic": ScopedBasic; "scoped-basic-root": ScopedBasicRoot; + "scoped-conditional": ScopedConditional; "scoped-slot-append-and-prepend": ScopedSlotAppendAndPrepend; "scoped-slot-child-insert-adjacent": ScopedSlotChildInsertAdjacent; "shadow-dom-array": ShadowDomArray; @@ -2261,6 +2275,7 @@ declare module "@stencil/core" { "sass-cmp": LocalJSX.SassCmp & JSXBase.HTMLAttributes; "scoped-basic": LocalJSX.ScopedBasic & JSXBase.HTMLAttributes; "scoped-basic-root": LocalJSX.ScopedBasicRoot & JSXBase.HTMLAttributes; + "scoped-conditional": LocalJSX.ScopedConditional & JSXBase.HTMLAttributes; "scoped-slot-append-and-prepend": LocalJSX.ScopedSlotAppendAndPrepend & JSXBase.HTMLAttributes; "scoped-slot-child-insert-adjacent": LocalJSX.ScopedSlotChildInsertAdjacent & JSXBase.HTMLAttributes; "shadow-dom-array": LocalJSX.ShadowDomArray & JSXBase.HTMLAttributes; diff --git a/test/karma/test-app/scoped-conditional/cmp.tsx b/test/karma/test-app/scoped-conditional/cmp.tsx new file mode 100644 index 00000000000..f0b40580351 --- /dev/null +++ b/test/karma/test-app/scoped-conditional/cmp.tsx @@ -0,0 +1,31 @@ +import { Component, h, Prop } from '@stencil/core'; + +@Component({ + tag: 'scoped-conditional', + scoped: true, +}) +export class ScopedConditional { + @Prop() renderHello: boolean = false; + + render() { + return ( +
+ {/* prior to fixing the bug */} + {/* - if you remove the conditional below, it works */} + {/* - if you remove the
around `.tag`, it works */} + {/* - if you add additional elements between the conditional and the second
, it works */} + + {/* Note: Need the conditional's first half, _and_ the innerHTML attr */} + {/* Interestingly, if we replace innerHTML with a text node as a child of the
, */} + {/* we get a separate error where the slot doesn't get put in the correct place */} + {this.renderHello &&
} + {/* This div below must be there too */} +
+ before slot-> + + <-after slot +
+
+ ); + } +} diff --git a/test/karma/test-app/scoped-conditional/index.html b/test/karma/test-app/scoped-conditional/index.html new file mode 100644 index 00000000000..4f823a9f143 --- /dev/null +++ b/test/karma/test-app/scoped-conditional/index.html @@ -0,0 +1,16 @@ + + + + + + + + + +
This div will be slotted in
+
diff --git a/test/karma/test-app/scoped-conditional/karma.spec.ts b/test/karma/test-app/scoped-conditional/karma.spec.ts new file mode 100644 index 00000000000..db0b621bf74 --- /dev/null +++ b/test/karma/test-app/scoped-conditional/karma.spec.ts @@ -0,0 +1,61 @@ +import { setupDomTests, waitForChanges } from '../util'; + +describe('scoped-conditional', () => { + const { setupDom, tearDownDom } = setupDomTests(document); + let app: HTMLElement | undefined; + + beforeEach(async () => { + app = await setupDom('/scoped-conditional/index.html'); + }); + + afterEach(tearDownDom); + + it('renders the initial slotted content', () => { + const host = app.querySelector('scoped-conditional'); + const outerDiv = host.querySelector('div'); + + expect(outerDiv.textContent).toBe( + `before slot-> + This div will be slotted in +<-after slot`, + ); + }); + + it('renders the slotted content after toggling the message', async () => { + // toggle the 'Hello' message, which should insert a new
into the DOM & _not_ remove the slotted content + const toggleButton = app.querySelector('#toggleHello'); + toggleButton.click(); + await waitForChanges(); + + const host = app.querySelector('scoped-conditional'); + const outerDivChildren = host.querySelector('div').childNodes; + + expect(outerDivChildren.length).toBe(2); + + expect(outerDivChildren[0].textContent).toBe('Hello'); + expect(outerDivChildren[1].textContent).toBe( + `before slot-> + This div will be slotted in +<-after slot`, + ); + }); + + it('renders the slotted content after toggling the twice message', async () => { + // toggle the 'Hello' message twice, which should insert a new
into the DOM, then remove it. + // as a result of the toggle, we should _not_ remove the slotted content + const toggleButton = app.querySelector('#toggleHello'); + toggleButton.click(); + await waitForChanges(); + toggleButton.click(); + await waitForChanges(); + + const host = app.querySelector('scoped-conditional'); + const outerDiv = host.querySelector('div'); + + expect(outerDiv.textContent).toBe( + `before slot-> + This div will be slotted in +<-after slot`, + ); + }); +}); diff --git a/test/karma/test-app/svg-attr/karma.spec.ts b/test/karma/test-app/svg-attr/karma.spec.ts index c2055699096..dcd1d655d18 100644 --- a/test/karma/test-app/svg-attr/karma.spec.ts +++ b/test/karma/test-app/svg-attr/karma.spec.ts @@ -10,16 +10,18 @@ describe('svg attr', () => { afterEach(tearDownDom); it('adds and removes attribute', async () => { - const rect = app.querySelector('rect'); + let rect = app.querySelector('rect'); expect(rect.getAttribute('transform')).toBe(null); const button = app.querySelector('button'); button.click(); await waitForChanges(); + rect = app.querySelector('rect'); expect(rect.getAttribute('transform')).toBe('rotate(45 27 27)'); button.click(); await waitForChanges(); + rect = app.querySelector('rect'); expect(rect.getAttribute('transform')).toBe(null); }); });