From eddc90363abe9058480c70d47952e402df2f3ab4 Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Thu, 10 Mar 2022 11:33:53 -0500 Subject: [PATCH 1/6] feat: create unified API for parsing markers --- .../fast-element/docs/api-report.md | 2 -- .../fast-element/src/templating/compiler.ts | 27 ++++++++-------- .../fast-element/src/templating/markup.ts | 31 ++++++------------- 3 files changed, 22 insertions(+), 38 deletions(-) diff --git a/packages/web-components/fast-element/docs/api-report.md b/packages/web-components/fast-element/docs/api-report.md index 61d4c91a073..cec0228ebce 100644 --- a/packages/web-components/fast-element/docs/api-report.md +++ b/packages/web-components/fast-element/docs/api-report.md @@ -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 diff --git a/packages/web-components/fast-element/src/templating/compiler.ts b/packages/web-components/fast-element/src/templating/compiler.ts index abbfdf8efe9..9a5624677e4 100644 --- a/packages/web-components/fast-element/src/templating/compiler.ts +++ b/packages/web-components/fast-element/src/templating/compiler.ts @@ -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,15 @@ function compileNode( return next; } +function isMarker(node: Node, directives: ReadonlyArray): boolean { + if (node && node.nodeType === Node.COMMENT_NODE) { + const result = Parser.parse((node as Comment).data, directives); + return result !== null; + } + + return false; +} + /** * Compiles a template and associated directives into a compilation * result which can be used to create views. @@ -284,7 +283,7 @@ 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!) || + isMarker(fragment.firstChild!, directives) || // Or if there is only one node, 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. diff --git a/packages/web-components/fast-element/src/templating/markup.ts b/packages/web-components/fast-element/src/templating/markup.ts index 8c66bcf54cd..14b2d7ad917 100644 --- 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 ``; - }, - - /** - * 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 ``; }, }); @@ -82,16 +69,16 @@ 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; @@ -99,16 +86,16 @@ export const Parser = Object.freeze({ 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; }, /** From f09a0627d9c467fd6666722d8ad9ec00b4e0ef9b Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Thu, 10 Mar 2022 16:46:26 -0500 Subject: [PATCH 2/6] fix: correct logic for inserting stable first node --- .../fast-element/src/components/controller.spec.ts | 2 +- .../web-components/fast-element/src/templating/compiler.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/web-components/fast-element/src/components/controller.spec.ts b/packages/web-components/fast-element/src/components/controller.spec.ts index 33bcd7a35da..953f2418c16 100644 --- 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 index 9a5624677e4..e5c3b48970e 100644 --- a/packages/web-components/fast-element/src/templating/compiler.ts +++ b/packages/web-components/fast-element/src/templating/compiler.ts @@ -284,10 +284,10 @@ export function compileTemplate( // 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!, directives) || - // Or if there is only one node, it means the template's content + // 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); } From e87f77caf1ad21f8eac507797d2babceed58d7d9 Mon Sep 17 00:00:00 2001 From: EisenbergEffect Date: Thu, 10 Mar 2022 16:47:34 -0500 Subject: [PATCH 3/6] Change files --- ...-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 change/@microsoft-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json diff --git a/change/@microsoft-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json b/change/@microsoft-fast-element-0e2b9162-79c1-43de-adf5-13989cd29ee1.json new file mode 100644 index 00000000000..95d0f81a395 --- /dev/null +++ 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": "roeisenb@microsoft.com", + "dependentChangeType": "patch" +} From 31e9b2b41682339f7dc74ef32e9479937b66ee8a Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Thu, 10 Mar 2022 16:52:34 -0500 Subject: [PATCH 4/6] refactor: tersify compiler comment first node check --- .../fast-element/src/templating/compiler.ts | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/packages/web-components/fast-element/src/templating/compiler.ts b/packages/web-components/fast-element/src/templating/compiler.ts index e5c3b48970e..17be84b08e3 100644 --- a/packages/web-components/fast-element/src/templating/compiler.ts +++ b/packages/web-components/fast-element/src/templating/compiler.ts @@ -235,12 +235,11 @@ function compileNode( } function isMarker(node: Node, directives: ReadonlyArray): boolean { - if (node && node.nodeType === Node.COMMENT_NODE) { - const result = Parser.parse((node as Comment).data, directives); - return result !== null; - } - - return false; + return ( + node && + node.nodeType == 8 && + Parser.parse((node as Comment).data, directives) !== null + ); } /** From 82273c7be63c7c5b1aebfe4aec3e244fdc2efca5 Mon Sep 17 00:00:00 2001 From: Rob Eisenberg Date: Thu, 10 Mar 2022 17:01:21 -0500 Subject: [PATCH 5/6] fix: correct some API types on the Parser --- packages/web-components/fast-element/docs/api-report.md | 9 ++++----- .../fast-element/src/templating/compiler.ts | 2 +- .../web-components/fast-element/src/templating/markup.ts | 4 ++-- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/web-components/fast-element/docs/api-report.md b/packages/web-components/fast-element/docs/api-report.md index cec0228ebce..7a6863579ec 100644 --- 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"; @@ -457,7 +457,7 @@ export const oneTime: BindingConfig & 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 @@ -500,14 +500,14 @@ export class RepeatBehavior implements Behavior, Subscriber { // @internal (undocumented) handleChange(source: any, args: Splice[]): void; unbind(): void; - } +} // @public export class RepeatDirective extends HTMLDirective { constructor(itemsBinding: Binding, templateBinding: Binding, options: RepeatOptions); createBehavior(targets: ViewBehaviorTargets): RepeatBehavior; createPlaceholder: (index: number) => string; - } +} // @public export interface RepeatOptions { @@ -654,7 +654,6 @@ export function volatile(target: {}, name: string | Accessor, descriptor: Proper // @public export function when(binding: Binding, templateOrTemplateBinding: SyntheticViewTemplate | Binding): CaptureType; - // (No @packageDocumentation comment for this package) ``` diff --git a/packages/web-components/fast-element/src/templating/compiler.ts b/packages/web-components/fast-element/src/templating/compiler.ts index 17be84b08e3..f2e57322d95 100644 --- 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, diff --git a/packages/web-components/fast-element/src/templating/markup.ts b/packages/web-components/fast-element/src/templating/markup.ts index 14b2d7ad917..327c9fe86d5 100644 --- a/packages/web-components/fast-element/src/templating/markup.ts +++ b/packages/web-components/fast-element/src/templating/markup.ts @@ -104,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; From 4f6db80a7084a1f32a9e9043c31b76cdbe7ab00d Mon Sep 17 00:00:00 2001 From: nicholasrice Date: Mon, 14 Mar 2022 09:52:31 -0700 Subject: [PATCH 6/6] change directive op type to use return type of Parser.aggregate() --- .../web-components/fast-ssr/src/template-parser/op-codes.ts | 6 +++--- .../fast-ssr/src/template-parser/template-parser.spec.ts | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) 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 index 669711ca992..18c1ee88d25 100644 --- 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,7 +62,7 @@ export type CustomElementShadowOp = { */ export type DirectiveOp = { type: OpType.directive; - directive: InlinableHTMLDirective; + directive: HTMLDirective; }; /** @@ -70,7 +70,7 @@ export type DirectiveOp = { */ 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 index dc50af0d681..c6287537b2f 100644 --- 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", () => {