Code review feedback.

`slot` instead of `slotName`. Fix up copy pasted tests such that they are more modern, remove unused pieces. Update the transformer tests.
lit · Dec 2, 2021 · 07417c2 · 07417c2
1 parent ad59fc3
commit 07417c2
Show file tree

Hide file tree

Showing 4 changed files with 53 additions and 69 deletions.
diff --git a/packages/reactive-element/src/decorators/query-assigned-elements.ts b/packages/reactive-element/src/decorators/query-assigned-elements.ts
@@ -18,7 +18,7 @@ export interface QueryAssignedElementsOptions extends AssignedNodesOptions {
   /**
    * A string name of the slot. Leave empty for the default slot.
    */
-  slotName?: string;
+  slot?: string;
   /**
    * A string which filters the results to elements that match the given css
    * selector.
@@ -35,15 +35,15 @@ export interface QueryAssignedElementsOptions extends AssignedNodesOptions {
  * @param options Object that sets options for nodes to be returned. See
  *     [MDN parameters section](https://developer.mozilla.org/en-US/docs/Web/API/HTMLSlotElement/assignedElements#parameters)
  *     for available options. Also accepts two more optional properties,
- *     `slotName` and `selector`.
- * @param options.slotName Name of the slot. Undefined or empty string for the
+ *     `slot` and `selector`.
+ * @param options.slot Name of the slot. Undefined or empty string for the
  *     default slot.
  * @param options.selector Element results are filtered such that they match the
  *     given css selector.
  *
  * ```ts
  * class MyElement {
- *   @queryAssignedElements({ slotName: 'list' })
+ *   @queryAssignedElements({ slot: 'list' })
  *   listItems;
  *   @queryAssignedElements()
  *   unnamedSlotEls;
@@ -59,15 +59,13 @@ export interface QueryAssignedElementsOptions extends AssignedNodesOptions {
  * @category Decorator
  */
 export function queryAssignedElements(options?: QueryAssignedElementsOptions) {
-  const {slotName, selector} = options ?? {};
+  const {slot, selector} = options ?? {};
   return decorateProperty({
     descriptor: (_name: PropertyKey) => ({
       get(this: ReactiveElement) {
-        const slotSelector = `slot${
-          slotName ? `[name=${slotName}]` : ':not([name])'
-        }`;
-        const slot = this.renderRoot?.querySelector(slotSelector);
-        const elements = (slot as HTMLSlotElement).assignedElements(options);
+        const slotSelector = `slot${slot ? `[name=${slot}]` : ':not([name])'}`;
+        const slotEl = this.renderRoot?.querySelector(slotSelector);
+        const elements = (slotEl as HTMLSlotElement).assignedElements(options);
         if (selector) {
           return elements.filter((node) => node.matches(selector));
         }

diff --git a/packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts b/packages/reactive-element/src/test/decorators/queryAssignedElements_test.ts
@@ -5,6 +5,7 @@
  */
 
 import {queryAssignedElements} from '../../decorators/query-assigned-elements.js';
+import {customElement} from '../../decorators/custom-element.js';
 import {
   canTestReactiveElement,
   generateElementName,
@@ -20,17 +21,18 @@ const flush =
   let container: HTMLElement;
   let el: C;
 
+  @customElement('assigned-elements-el')
   class D extends RenderingElement {
     @queryAssignedElements() defaultAssigned!: Element[];
 
-    @queryAssignedElements({slotName: 'footer', flatten: true})
+    @queryAssignedElements({slot: 'footer', flatten: true})
     footerAssigned!: Element[];
 
-    @queryAssignedElements({slotName: 'footer', flatten: false})
+    @queryAssignedElements({slot: 'footer', flatten: false})
     footerNotFlattenedSlot!: Element[];
 
     @queryAssignedElements({
-      slotName: 'footer',
+      slot: 'footer',
       flatten: true,
       selector: '.item',
     })
@@ -43,12 +45,12 @@ const flush =
       `;
     }
   }
-  customElements.define('assigned-elements-el', D);
 
+  @customElement('assigned-elements-el-2')
   class E extends RenderingElement {
     @queryAssignedElements() defaultAssigned!: Element[];
 
-    @queryAssignedElements({slotName: 'header'}) headerAssigned!: Element[];
+    @queryAssignedElements({slot: 'header'}) headerAssigned!: Element[];
 
     override render() {
       return html`
@@ -57,26 +59,21 @@ const flush =
       `;
     }
   }
-  customElements.define('assigned-elements-el-2', E);
 
   const defaultSymbol = Symbol('default');
-  const headerSymbol = Symbol('header');
+
+  @customElement('assigned-elements-el-symbol')
   class S extends RenderingElement {
     @queryAssignedElements() [defaultSymbol]!: Element[];
 
-    @queryAssignedElements({slotName: 'header'}) [headerSymbol]!: Element[];
-
     override render() {
-      return html`
-        <slot name="header"></slot>
-        <slot></slot>
-      `;
+      return html` <slot></slot> `;
     }
   }
-  customElements.define('assigned-elements-el-symbol', S);
 
   // Note, there are 2 elements here so that the `flatten` option of
   // the decorator can be tested.
+  @customElement(generateElementName())
   class C extends RenderingElement {
     div!: HTMLDivElement;
     div2!: HTMLDivElement;
@@ -114,23 +111,17 @@ const flush =
       ) as S;
     }
   }
-  customElements.define(generateElementName(), C);
 
   setup(async () => {
     container = document.createElement('div');
-    container.id = 'test-container';
-    document.body.appendChild(container);
+    document.body.append(container);
     el = new C();
-    container.appendChild(el);
-    await el.updateComplete;
-    await el.assignedEls.updateComplete;
+    container.append(el);
+    await new Promise((r) => setTimeout(r, 0));
   });
 
   teardown(() => {
-    if (container !== undefined) {
-      container.parentElement!.removeChild(container);
-      (container as any) = undefined;
-    }
+    container?.remove();
   });
 
   test('returns assignedElements for slot', () => {
@@ -139,13 +130,12 @@ const flush =
     assert.deepEqual(el.assignedEls.defaultAssigned, [el.div]);
     const child = document.createElement('div');
     const text1 = document.createTextNode('');
-    el.assignedEls.appendChild(text1);
-    el.assignedEls.appendChild(child);
+    el.assignedEls.append(text1, child);
     const text2 = document.createTextNode('');
-    el.assignedEls.appendChild(text2);
+    el.assignedEls.append(text2);
     flush();
     assert.deepEqual(el.assignedEls.defaultAssigned, [el.div, child]);
-    el.assignedEls.removeChild(child);
+    child.remove();
     flush();
     assert.deepEqual(el.assignedEls.defaultAssigned, [el.div]);
   });
@@ -162,15 +152,14 @@ const flush =
     assert.deepEqual(el.assignedEls.footerAssigned, []);
     const child1 = document.createElement('div');
     const child2 = document.createElement('div');
-    el.appendChild(child1);
-    el.appendChild(child2);
+    el.append(child1, child2);
     flush();
     assert.deepEqual(el.assignedEls.footerAssigned, [child1, child2]);
 
     assert.equal(el.assignedEls.footerNotFlattenedSlot.length, 1);
     assert.equal(el.assignedEls.footerNotFlattenedSlot?.[0]?.tagName, 'SLOT');
 
-    el.removeChild(child2);
+    child2.remove();
     flush();
     assert.deepEqual(el.assignedEls.footerAssigned, [child1]);
   });
@@ -184,11 +173,10 @@ const flush =
     const child1 = document.createElement('div');
     const child2 = document.createElement('div');
     child2.classList.add('item');
-    el.appendChild(child1);
-    el.appendChild(child2);
+    el.append(child1, child2);
     flush();
     assert.deepEqual(el.assignedEls.footerAssignedFiltered, [child2]);
-    el.removeChild(child2);
+    child2.remove();
     flush();
     assert.deepEqual(el.assignedEls.footerAssignedFiltered, []);
   });

diff --git a/packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts b/packages/ts-transformers/src/internal/decorators/query-assigned-elements.ts
@@ -12,7 +12,7 @@ import type {MemberDecoratorVisitor} from '../visitor.js';
 /**
  * Transform:
  *
- *   @queryAssignedElements({slotName: 'list', selector: '.item'})
+ *   @queryAssignedElements({slot: 'list', selector: '.item'})
  *   listItems
  *
  * Into:
@@ -59,28 +59,28 @@ export class QueryAssignedElementsVisitor implements MemberDecoratorVisitor {
     if (arg0 && arg0.properties.some((p) => !ts.isPropertyAssignment(p))) {
       throw new Error(
         `queryAssignedElements object literal argument can only include ` +
-          `property assignment. For example: '{ slotName: "example" }' is ` +
+          `property assignment. For example: '{ slot: "example" }' is ` +
           `supported, whilst '{ ...otherOpts }' is unsupported.`
       );
     }
-    const {slotName, selector} = this._retrieveSlotAndSelector(arg0);
+    const {slot, selector} = this._retrieveSlotAndSelector(arg0);
     litClassContext.litFileContext.replaceAndMoveComments(
       property,
       this._createQueryAssignedElementsGetter(
         name,
-        slotName,
+        slot,
         selector,
         this._filterAssignedElementsOptions(arg0)
       )
     );
   }
 
   private _retrieveSlotAndSelector(opts?: ts.ObjectLiteralExpression): {
-    slotName: string;
+    slot: string;
     selector: string;
   } {
     if (!opts) {
-      return {slotName: '', selector: ''};
+      return {slot: '', selector: ''};
     }
     const findStringLiteralFor = (key: string): string => {
       const propAssignment = opts.properties.find(
@@ -102,7 +102,7 @@ export class QueryAssignedElementsVisitor implements MemberDecoratorVisitor {
       );
     };
     return {
-      slotName: findStringLiteralFor('slotName'),
+      slot: findStringLiteralFor('slot'),
       selector: findStringLiteralFor('selector'),
     };
   }
@@ -116,7 +116,7 @@ export class QueryAssignedElementsVisitor implements MemberDecoratorVisitor {
    * Given:
    *
    * ```ts
-   * { slotName: 'example', flatten: false }
+   * { slot: 'example', flatten: false }
    * ```
    *
    * returns:
@@ -139,7 +139,7 @@ export class QueryAssignedElementsVisitor implements MemberDecoratorVisitor {
       (p) =>
         p.name &&
         ts.isIdentifier(p.name) &&
-        !['slotName', 'selector'].includes(p.name.text)
+        !['slot', 'selector'].includes(p.name.text)
     );
     if (assignedElementsProperties.length === 0) {
       return;
@@ -152,15 +152,13 @@ export class QueryAssignedElementsVisitor implements MemberDecoratorVisitor {
 
   private _createQueryAssignedElementsGetter(
     name: string,
-    slotName: string,
+    slot: string,
     selector: string,
     assignedElsOptions?: ts.ObjectLiteralExpression
   ) {
     const factory = this._factory;
 
-    const slotSelector = `slot${
-      slotName ? `[name=${slotName}]` : ':not([name])'
-    }`;
+    const slotSelector = `slot${slot ? `[name=${slot}]` : ':not([name])'}`;
 
     const assignedElementsOptions = assignedElsOptions
       ? [assignedElsOptions]

diff --git a/packages/ts-transformers/src/tests/idiomatic-decorators-test.ts b/packages/ts-transformers/src/tests/idiomatic-decorators-test.ts
@@ -605,7 +605,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({ slotName: 'list' })
+      @queryAssignedElements({ slot: 'list' })
       listItems: HTMLElement[];
     }
     `;
@@ -632,7 +632,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({ slotName: 'list', flatten: true })
+      @queryAssignedElements({ slot: 'list', flatten: true })
       listItems: NodeListOf<HTMLElement>;
     }
     `;
@@ -659,7 +659,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({slotName: 'list', flatten: false, selector: '.item'})
+      @queryAssignedElements({slot: 'list', flatten: false, selector: '.item'})
       listItems: NodeListOf<HTMLElement>;
     }
     `;
@@ -692,7 +692,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({slotName: 'list', flatten: isFlatten, selector: '.item'})
+      @queryAssignedElements({slot: 'list', flatten: isFlatten, selector: '.item'})
       listItems: HTMLElement[];
     }
     `;
@@ -721,7 +721,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
     import {LitElement} from 'lit';
     import {queryAssignedElements} from 'lit/decorators.js';
 
-    const someIdentifier = {slotName: 'list'};
+    const someIdentifier = {slot: 'list'};
 
     class MyElement extends LitElement {
       // listItems comment
@@ -742,7 +742,7 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({slotName: 'list', ...{}})
+      @queryAssignedElements({slot: 'list', ...{}})
       listItems: HTMLElement[];
     }
     `;
@@ -757,11 +757,11 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
     import {LitElement} from 'lit';
     import {queryAssignedElements} from 'lit/decorators.js';
 
-    const slotName = "shorthandSyntaxInvalid";
+    const slot = "shorthandSyntaxInvalid";
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({slotName})
+      @queryAssignedElements({slot})
       listItems: HTMLElement[];
     }
     `;
@@ -771,22 +771,22 @@ const tests = (test: uvu.Test<uvu.Context>, options: ts.CompilerOptions) => {
     );
   });
 
-  test('@queryAssignedElements (fail if slotName or selector not literal)', () => {
+  test('@queryAssignedElements (fail if slot or selector not literal)', () => {
     const input = `
     import {LitElement} from 'lit';
     import {queryAssignedElements} from 'lit/decorators.js';
 
-    const slotName = 'list';
+    const slot = 'list';
 
     class MyElement extends LitElement {
       // listItems comment
-      @queryAssignedElements({slotName: slotName})
+      @queryAssignedElements({slot: slot})
       listItems: HTMLElement[];
     }
     `;
     assert.throws(
       () => checkTransform(input, '', options),
-      /property 'slotName' must be a string literal/
+      /property 'slot' must be a string literal/
     );
   });