feat: create unified API for parsing markers (#5727)

* feat: create unified API for parsing markers * fix: correct logic for inserting stable first node * Change files * refactor: tersify compiler comment first node check * fix: correct some API types on the Parser * change directive op type to use return type of Parser.aggregate() Co-authored-by: EisenbergEffect <[email protected]> Co-authored-by: nicholasrice <[email protected]>
microsoft · May 3, 2022 · db13323 · db13323
1 parent 58b4c9d
commit db13323
Show file tree

Hide file tree

Showing 7 changed files with 41 additions and 53 deletions.
diff --git a/change/@microsoft-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json b/change/@microsoft-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json
@@ -0,0 +1,7 @@
+{
+  "type": "major",
+  "comment": "feat: create unified API for parsing markers",
+  "packageName": "@microsoft/fast-element",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}
diff --git a/packages/web-components/fast-element/docs/api-report.md b/packages/web-components/fast-element/docs/api-report.md
@@ -49,7 +49,7 @@ export class AttributeDefinition implements Accessor {
     onAttributeChangedCallback(element: HTMLElement, value: any): void;
     readonly Owner: Function;
     setValue(source: HTMLElement, newValue: any): void;
-    }
+}
 
 // @public
 export type AttributeMode = "reflect" | "boolean" | "fromView";
@@ -395,11 +395,9 @@ export const enum KernelServiceId {
 
 // @public
 export const Markup: Readonly<{
-    marker: string;
     interpolation(index: number): string;
     attribute(index: number): string;
     comment(index: number): string;
-    indexFromComment(node: Comment): number;
 }>;
 
 // Warning: (ae-internal-missing-underscore) The name "Mutable" should be prefixed with an underscore because the declaration is marked as @internal
@@ -459,7 +457,7 @@ export const oneTime: BindingConfig<DefaultBindingOptions> & BindingConfigResolv
 // @public
 export const Parser: Readonly<{
     parse(value: string, directives: readonly HTMLDirective[]): (string | HTMLDirective)[] | null;
-    aggregate(parts: (string | HTMLDirective)[]): InlinableHTMLDirective;
+    aggregate(parts: (string | HTMLDirective)[]): HTMLDirective;
 }>;
 
 // @public
@@ -502,14 +500,14 @@ export class RepeatBehavior<TSource = any> implements Behavior, Subscriber {
     // @internal (undocumented)
     handleChange(source: any, args: Splice[]): void;
     unbind(): void;
-    }
+}
 
 // @public
 export class RepeatDirective<TSource = any> extends HTMLDirective {
     constructor(itemsBinding: Binding, templateBinding: Binding<TSource, SyntheticViewTemplate>, options: RepeatOptions);
     createBehavior(targets: ViewBehaviorTargets): RepeatBehavior<TSource>;
     createPlaceholder: (index: number) => string;
-    }
+}
 
 // @public
 export interface RepeatOptions {
@@ -656,7 +654,6 @@ export function volatile(target: {}, name: string | Accessor, descriptor: Proper
 // @public
 export function when<TSource = any, TReturn = any>(binding: Binding<TSource, TReturn>, templateOrTemplateBinding: SyntheticViewTemplate | Binding<TSource, SyntheticViewTemplate>): CaptureType<TSource>;
 
-
 // (No @packageDocumentation comment for this package)
 
 ```
diff --git a/packages/web-components/fast-element/src/components/controller.spec.ts b/packages/web-components/fast-element/src/components/controller.spec.ts
@@ -531,7 +531,7 @@ describe("The Controller", () => {
                 }
             ).define();
 
-            expect(root.innerHTML).to.equal("<!---->Test 2");
+            expect(root.innerHTML).to.equal("Test 2");
 
             document.body.removeChild(element);
         });

diff --git a/packages/web-components/fast-element/src/templating/compiler.ts b/packages/web-components/fast-element/src/templating/compiler.ts
@@ -1,6 +1,6 @@
 import { isString } from "../interfaces.js";
 import { DOM } from "../dom.js";
-import { Markup, Parser } from "./markup.js";
+import { Parser } from "./markup.js";
 import { bind, oneTime } from "./binding.js";
 import type {
     AspectedHTMLDirective,
@@ -111,12 +111,6 @@ class CompilationContext implements HTMLTemplateCompilationResult {
     }
 }
 
-const marker = Markup.marker;
-
-function isMarker(node: Node): node is Comment {
-    return node && node.nodeType === 8 && (node as Comment).data.startsWith(marker);
-}
-
 function compileAttributes(
     context: CompilationContext,
     parentId: string,
@@ -228,13 +222,9 @@ function compileNode(
         case 3: // text node
             return compileContent(context, node as Text, parentId, nodeId, nodeIndex);
         case 8: // comment
-            if (isMarker(node)) {
-                context.addFactory(
-                    context.directives[Markup.indexFromComment(node)],
-                    parentId,
-                    nodeId,
-                    nodeIndex
-                );
+            const parts = Parser.parse((node as Comment).data, context.directives);
+            if (parts !== null) {
+                context.addFactory(Parser.aggregate(parts), parentId, nodeId, nodeIndex);
             }
             break;
     }
@@ -244,6 +234,14 @@ function compileNode(
     return next;
 }
 
+function isMarker(node: Node, directives: ReadonlyArray<HTMLDirective>): boolean {
+    return (
+        node &&
+        node.nodeType == 8 &&
+        Parser.parse((node as Comment).data, directives) !== null
+    );
+}
+
 /**
  * Compiles a template and associated directives into a compilation
  * result which can be used to create views.
@@ -284,11 +282,11 @@ export function compileTemplate(
         // because something like a when, repeat, etc. could add nodes before the marker.
         // To mitigate this, we insert a stable first node. However, if we insert a node,
         // that will alter the result of the TreeWalker. So, we also need to offset the target index.
-        isMarker(fragment.firstChild!) ||
-        // Or if there is only one node, it means the template's content
+        isMarker(fragment.firstChild!, directives) ||
+        // Or if there is only one node and a directive, it means the template's content
         // is *only* the directive. In that case, HTMLView.dispose() misses any nodes inserted by
         // the directive. Inserting a new node ensures proper disposal of nodes added by the directive.
-        fragment.childNodes.length === 1
+        (fragment.childNodes.length === 1 && directives.length)
     ) {
         fragment.insertBefore(document.createComment(""), fragment.firstChild);
     }

diff --git a/packages/web-components/fast-element/src/templating/markup.ts b/packages/web-components/fast-element/src/templating/markup.ts
@@ -18,11 +18,6 @@ export const nextId = (): string => `${marker}-${++id}`;
  * @public
  */
 export const Markup = Object.freeze({
-    /**
-     * Gets the unique marker used by FAST to annotate templates.
-     */
-    marker,
-
     /**
      * Creates a placeholder string suitable for marking out a location *within*
      * an attribute value or HTML content.
@@ -53,15 +48,7 @@ export const Markup = Object.freeze({
      * Used internally by structural directives such as `repeat`.
      */
     comment(index: number): string {
-        return `<!--${marker}:${index}-->`;
-    },
-
-    /**
-     * Given a marker node, extract the {@link HTMLDirective} index from the placeholder.
-     * @param node - The marker node to extract the index from.
-     */
-    indexFromComment(node: Comment): number {
-        return parseInt(node.data.replace(`${marker}:`, ""));
+        return `<!--${interpolationStart}${index}${interpolationEnd}-->`;
     },
 });
 
@@ -82,33 +69,33 @@ export const Parser = Object.freeze({
         value: string,
         directives: readonly HTMLDirective[]
     ): (string | HTMLDirective)[] | null {
-        const valueParts = value.split(interpolationStart);
+        const parts = value.split(interpolationStart);
 
-        if (valueParts.length === 1) {
+        if (parts.length === 1) {
             return null;
         }
 
-        const bindingParts: (string | HTMLDirective)[] = [];
+        const result: (string | HTMLDirective)[] = [];
 
-        for (let i = 0, ii = valueParts.length; i < ii; ++i) {
-            const current = valueParts[i];
+        for (let i = 0, ii = parts.length; i < ii; ++i) {
+            const current = parts[i];
             const index = current.indexOf(interpolationEnd);
             let literal: string | HTMLDirective;
 
             if (index === -1) {
                 literal = current;
             } else {
                 const directiveIndex = parseInt(current.substring(0, index));
-                bindingParts.push(directives[directiveIndex]);
+                result.push(directives[directiveIndex]);
                 literal = current.substring(index + interpolationEndLength);
             }
 
             if (literal !== "") {
-                bindingParts.push(literal);
+                result.push(literal);
             }
         }
 
-        return bindingParts;
+        return result;
     },
 
     /**
@@ -117,9 +104,9 @@ export const Parser = Object.freeze({
      * directives.
      * @returns A single inline directive that aggregates the behavior of all the parts.
      */
-    aggregate(parts: (string | HTMLDirective)[]): InlinableHTMLDirective {
+    aggregate(parts: (string | HTMLDirective)[]): HTMLDirective {
         if (parts.length === 1) {
-            return parts[0] as InlinableHTMLDirective;
+            return parts[0] as HTMLDirective;
         }
 
         let aspect: string | undefined;

diff --git a/packages/web-components/fast-ssr/src/template-parser/op-codes.ts b/packages/web-components/fast-ssr/src/template-parser/op-codes.ts
@@ -1,4 +1,4 @@
-import { InlinableHTMLDirective } from "@microsoft/fast-element";
+import { HTMLDirective } from "@microsoft/fast-element";
 import { AttributeType } from "./attributes.js";
 
 /**
@@ -62,15 +62,15 @@ export type CustomElementShadowOp = {
  */
 export type DirectiveOp = {
     type: OpType.directive;
-    directive: InlinableHTMLDirective;
+    directive: HTMLDirective;
 };
 
 /**
  * Operation to emit a bound attribute
  */
 export type AttributeBindingOp = {
     type: OpType.attributeBinding;
-    directive: InlinableHTMLDirective;
+    directive: HTMLDirective;
     name: string;
     attributeType: AttributeType;
     useCustomElementInstance: boolean;

diff --git a/packages/web-components/fast-ssr/src/template-parser/template-parser.spec.ts b/packages/web-components/fast-ssr/src/template-parser/template-parser.spec.ts
@@ -38,7 +38,6 @@ test.describe("parseTemplateToOpCodes", () => {
             const code = codes[0] as DirectiveOp;
             expect(codes.length).toBe(1);
             expect(code.type).toBe(OpType.directive);
-            expect(code.directive.binding(null, defaultExecutionContext)).toBe("Hello World.")
     });
     test("should sandwich directive ops between text ops when binding native element content", () => {