diff --git a/change/@microsoft-fast-element-24c750b0-357e-414e-8e48-588822f6822d.json b/change/@microsoft-fast-element-24c750b0-357e-414e-8e48-588822f6822d.json new file mode 100644 index 00000000000..3d71dc51732 --- /dev/null +++ b/change/@microsoft-fast-element-24c750b0-357e-414e-8e48-588822f6822d.json @@ -0,0 +1,7 @@ +{ + "type": "prerelease", + "comment": "fix: prevent circular reference errors when stringifying DOM nodes controlled by FAST's rendering engine", + "packageName": "@microsoft/fast-element", + "email": "roeisenb@microsoft.com", + "dependentChangeType": "prerelease" +} diff --git a/packages/web-components/fast-element/docs/api-report.md b/packages/web-components/fast-element/docs/api-report.md index b70cb43f838..ac3d1c41cd0 100644 --- a/packages/web-components/fast-element/docs/api-report.md +++ b/packages/web-components/fast-element/docs/api-report.md @@ -262,6 +262,8 @@ export class ElementController exten readonly source: TElement; get template(): ElementViewTemplate | null; set template(value: ElementViewTemplate | null); + // @internal + toJSON: () => undefined; readonly view: ElementView | null; } @@ -518,6 +520,8 @@ export class HTMLView implements ElementView undefined; unbind(): void; } @@ -758,6 +762,8 @@ export abstract class StatelessAttachedAttributeDirective implements H nodeId: string; // (undocumented) protected options: TOptions; + // @internal + toJSON: () => undefined; } // @public @@ -908,6 +914,8 @@ export class ViewTemplate implements ElementViewTe readonly factories: Record; readonly html: string | HTMLTemplateElement; render(source: TSource, host: Node, hostBindingTarget?: Element): HTMLView; + // @internal + toJSON: () => undefined; } // @public diff --git a/packages/web-components/fast-element/src/components/element-controller.spec.ts b/packages/web-components/fast-element/src/components/element-controller.spec.ts index 970fa94d861..d7a1ef16d19 100644 --- a/packages/web-components/fast-element/src/components/element-controller.spec.ts +++ b/packages/web-components/fast-element/src/components/element-controller.spec.ts @@ -634,4 +634,12 @@ describe("The ElementController", () => { expect(order[6]).to.equal('parent behavior disconnected'); expect(order[7]).to.equal('child behavior disconnected'); }); + + it("should not throw if DOM stringified", () => { + const controller = createController(); + + expect(() => { + JSON.stringify(controller.element); + }).to.not.throw(); + }); }); diff --git a/packages/web-components/fast-element/src/components/element-controller.ts b/packages/web-components/fast-element/src/components/element-controller.ts index 42cb7036873..984c39cbb13 100644 --- a/packages/web-components/fast-element/src/components/element-controller.ts +++ b/packages/web-components/fast-element/src/components/element-controller.ts @@ -1,4 +1,4 @@ -import { Message, Mutable, StyleStrategy, StyleTarget } from "../interfaces.js"; +import { Message, Mutable, noop, StyleStrategy, StyleTarget } from "../interfaces.js"; import { PropertyChangeNotifier } from "../observation/notifier.js"; import { Observable, SourceLifetime } from "../observation/observable.js"; import { FAST } from "../platform.js"; @@ -438,6 +438,12 @@ export class ElementController return false; } + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; + private renderTemplate(template: ElementViewTemplate | null | undefined): void { // When getting the host to render to, we start by looking // up the shadow root. If there isn't one, then that means diff --git a/packages/web-components/fast-element/src/observation/observable.ts b/packages/web-components/fast-element/src/observation/observable.ts index 7541c293d60..b86b9344912 100644 --- a/packages/web-components/fast-element/src/observation/observable.ts +++ b/packages/web-components/fast-element/src/observation/observable.ts @@ -4,6 +4,7 @@ import { isString, KernelServiceId, Message, + noop, } from "../interfaces.js"; import { createMetadataLocator, FAST } from "../platform.js"; import { Updates } from "./update-queue.js"; @@ -254,6 +255,11 @@ export const Observable = FAST.getById(KernelServiceId.observable, () => { super(expression, initialSubscriber); } + /** + * Opts out of JSON stringification. + */ + toJSON = noop; + public setMode(isAsync: boolean): void { this.isAsync = this.needsQueue = isAsync; } diff --git a/packages/web-components/fast-element/src/templating/binding-signal.ts b/packages/web-components/fast-element/src/templating/binding-signal.ts index 11fff9c2ea8..dfced5c987f 100644 --- a/packages/web-components/fast-element/src/templating/binding-signal.ts +++ b/packages/web-components/fast-element/src/templating/binding-signal.ts @@ -3,7 +3,7 @@ import type { ExpressionController, ExpressionObserver, } from "../observation/observable.js"; -import { isString } from "../interfaces.js"; +import { isString, noop } from "../interfaces.js"; import type { Subscriber } from "../observation/notifier.js"; import type { HTMLBindingDirective } from "./binding.js"; import { Binding } from "./html-directive.js"; @@ -78,6 +78,12 @@ class SignalObserver implements Sub this.subscriber.handleChange(this.dataBinding.evaluate, this); } + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; + private getSignal(controller: ExpressionController): string { const options = this.dataBinding.options; return isString(options) diff --git a/packages/web-components/fast-element/src/templating/binding-two-way.ts b/packages/web-components/fast-element/src/templating/binding-two-way.ts index 03881de2344..0fa38a0d7cc 100644 --- a/packages/web-components/fast-element/src/templating/binding-two-way.ts +++ b/packages/web-components/fast-element/src/templating/binding-two-way.ts @@ -1,4 +1,4 @@ -import { isString, Message } from "../interfaces.js"; +import { isString, Message, noop } from "../interfaces.js"; import type { Subscriber } from "../observation/notifier.js"; import { ExecutionContext, @@ -133,6 +133,12 @@ class TwoWayObserver value ); } + + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; } class TwoWayBinding extends Binding< diff --git a/packages/web-components/fast-element/src/templating/binding.spec.ts b/packages/web-components/fast-element/src/templating/binding.spec.ts index 8a121ef4712..0c2ab583774 100644 --- a/packages/web-components/fast-element/src/templating/binding.spec.ts +++ b/packages/web-components/fast-element/src/templating/binding.spec.ts @@ -5,7 +5,7 @@ import { html, ViewTemplate } from "./template.js"; import { toHTML } from "../__test__/helpers.js"; import { SyntheticView, HTMLView } from "./view.js"; import { Updates } from "../observation/update-queue.js"; -import { Aspect, HTMLDirective } from "./html-directive.js"; +import { Aspect } from "./html-directive.js"; import { DOM } from "./dom.js"; import { Signal, signal } from "./binding-signal.js"; import { twoWay, TwoWayBindingOptions } from "./binding-two-way.js"; @@ -120,6 +120,20 @@ describe("The HTML binding directive", () => { expect(node.textContent).to.equal(model.value); }); + + it("should not throw if DOM stringified", () => { + const { behavior, node, targets } = contentBinding(); + const model = new Model("This is a test"); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(node.textContent).to.equal(model.value); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); }); context("when binding template content", () => { @@ -284,6 +298,20 @@ describe("The HTML binding directive", () => { expect(toHTML(parentNode)).to.equal(`Hi there!`); }); + + it("target node should not stringify $fastView or $fastTemplate", () => { + const { behavior, node, targets } = contentBinding(); + const template = html`This is a template. ${x => x.knownValue}`; + const model = new Model(template); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + const clone = JSON.parse(JSON.stringify(node)); + + expect("$fastView" in clone).to.be.false; + expect("$fastTemplate" in clone).to.be.false; + }); }) context("when unbinding template content", () => { @@ -423,6 +451,18 @@ describe("The HTML binding directive", () => { expect(aspectScenario.getValue(node)).to.equal(aspectScenario.originalValue); }); + + it("should not throw if DOM stringified", () => { + const { behavior, node, targets } = defaultBinding(aspectScenario.sourceAspect); + const model = new Model(aspectScenario.originalValue); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); } }); @@ -469,6 +509,18 @@ describe("The HTML binding directive", () => { expect(aspectScenario.getValue(node)).to.equal(aspectScenario.originalValue); }); + + it("should not throw if DOM stringified", () => { + const { behavior, node, targets } = oneTimeBinding(aspectScenario.sourceAspect); + const model = new Model(aspectScenario.originalValue); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); } }); @@ -525,6 +577,18 @@ describe("The HTML binding directive", () => { expect(aspectScenario.getValue(node)).to.equal(aspectScenario.originalValue); }); + + it("should not throw if DOM stringified", () => { + const { behavior, node, targets } = signalBinding("test-signal", aspectScenario.sourceAspect); + const model = new Model(aspectScenario.originalValue); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); } }); @@ -624,6 +688,18 @@ describe("The HTML binding directive", () => { expect(aspectScenario.getValue(node)).to.equal(aspectScenario.originalValue); }); + + it("should not throw if DOM stringified", () => { + const { behavior, node, targets } = twoWayBinding({}, aspectScenario.sourceAspect); + const model = new Model(aspectScenario.originalValue); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); } }); @@ -686,6 +762,18 @@ describe("The HTML binding directive", () => { node.dispatchEvent(new CustomEvent("my-event")); expect(model.actionInvokeCount).to.equal(1); }); + + it("should not throw if DOM stringified", () => { + const { behavior, targets, node } = eventBinding({}, "@my-event"); + const model = new Model("Test value."); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); }); context('when binding classList', () => { @@ -749,5 +837,20 @@ describe("The HTML binding directive", () => { updateTarget(element, observerA, undefined); expect(contains('foo')).false; }); + + it("should not throw if DOM stringified", () => { + const directive = new HTMLBindingDirective(bind(() => "")); + const { behavior, node, targets } = configureDirective(directive, ":classList"); + + Aspect.assign(directive, ":classList"); + const model = new Model("Test value."); + const controller = Fake.viewController(targets, behavior); + + controller.bind(model); + + expect(() => { + JSON.stringify(node); + }).to.not.throw(); + }); }); }); diff --git a/packages/web-components/fast-element/src/templating/binding.ts b/packages/web-components/fast-element/src/templating/binding.ts index 0090c7dde20..f4f510a0f18 100644 --- a/packages/web-components/fast-element/src/templating/binding.ts +++ b/packages/web-components/fast-element/src/templating/binding.ts @@ -1,5 +1,5 @@ import type { Subscriber } from "../index.js"; -import { isFunction, Message } from "../interfaces.js"; +import { isFunction, Message, noop } from "../interfaces.js"; import { ExecutionContext, Expression, @@ -57,6 +57,12 @@ class OneTimeBinding bind(controller: ExpressionController): TReturn { return this.evaluate(controller.source, controller.context); } + + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; } type UpdateTarget = ( diff --git a/packages/web-components/fast-element/src/templating/children.spec.ts b/packages/web-components/fast-element/src/templating/children.spec.ts index b14f01ea2f0..ec378d720f4 100644 --- a/packages/web-components/fast-element/src/templating/children.spec.ts +++ b/packages/web-components/fast-element/src/templating/children.spec.ts @@ -3,8 +3,9 @@ import { children, ChildrenDirective } from "./children.js"; import { observable } from "../observation/observable.js"; import { elements } from "./node-observation.js"; import { Updates } from "../observation/update-queue.js"; -import type { ViewBehaviorTargets, ViewController } from "./html-directive.js"; import { Fake } from "../testing/fakes.js"; +import { html } from "./template.js"; +import { ref } from "./ref.js"; describe("The children", () => { context("template function", () => { @@ -25,6 +26,7 @@ describe("The children", () => { context("behavior", () => { class Model { @observable nodes; + reference: HTMLElement; } function createAndAppendChildren(host: HTMLElement, elementName = "div") { @@ -48,18 +50,6 @@ describe("The children", () => { return { host, children, targets, nodeId }; } - function createController(source: any, targets: ViewBehaviorTargets): ViewController { - return { - source, - targets, - context: Fake.executionContext(), - isBound: false, - onUnbind() { - - } - }; - } - it("gathers child nodes", () => { const { host, children, targets, nodeId } = createDOM(); const behavior = new ChildrenDirective({ @@ -67,9 +57,9 @@ describe("The children", () => { }); behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(children); }); @@ -82,9 +72,9 @@ describe("The children", () => { }); behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(children.filter(elements("foo-bar"))); }); @@ -96,9 +86,9 @@ describe("The children", () => { }); behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(children); @@ -117,9 +107,9 @@ describe("The children", () => { }); behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(children); @@ -151,9 +141,9 @@ describe("The children", () => { behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(subtreeChildren); @@ -179,9 +169,9 @@ describe("The children", () => { }); behavior.nodeId = nodeId; const model = new Model(); - const controller = createController(model, targets); + const controller = Fake.viewController(targets, behavior); - behavior.bind(controller); + controller.bind(model); expect(model.nodes).members(children); @@ -195,5 +185,25 @@ describe("The children", () => { expect(model.nodes).members([]); }); + + it("should not throw if DOM stringified", () => { + const template = html` +
+
+ `; + + const view = template.create(); + const model = new Model(); + + view.bind(model); + + expect(() => { + JSON.stringify(model.reference); + }).to.not.throw(); + + view.unbind(); + }); }); }); diff --git a/packages/web-components/fast-element/src/templating/children.ts b/packages/web-components/fast-element/src/templating/children.ts index 1133ea7985a..709714309eb 100644 --- a/packages/web-components/fast-element/src/templating/children.ts +++ b/packages/web-components/fast-element/src/templating/children.ts @@ -1,4 +1,4 @@ -import { isString } from "../interfaces.js"; +import { isString, noop } from "../interfaces.js"; import { HTMLDirective } from "./html-directive.js"; import { NodeBehaviorOptions, NodeObservationDirective } from "./node-observation.js"; import type { CaptureType } from "./template.js"; @@ -61,10 +61,15 @@ export class ChildrenDirective extends NodeObservationDirective< * @param target - The target to observe. */ observe(target: any): void { - const observer = - target[this.observerProperty] ?? - (target[this.observerProperty] = new MutationObserver(this.handleEvent)); - observer.target = target; + let observer = target[this.observerProperty]; + + if (!observer) { + observer = new MutationObserver(this.handleEvent); + observer.toJSON = noop; + observer.target = target; + target[this.observerProperty] = observer; + } + observer.observe(target, this.options); } diff --git a/packages/web-components/fast-element/src/templating/html-directive.ts b/packages/web-components/fast-element/src/templating/html-directive.ts index 35da0e4dbb1..ab9fe17b7be 100644 --- a/packages/web-components/fast-element/src/templating/html-directive.ts +++ b/packages/web-components/fast-element/src/templating/html-directive.ts @@ -1,5 +1,5 @@ import type { HostBehavior } from "../index.js"; -import type { Constructable, Mutable } from "../interfaces.js"; +import { Constructable, Mutable, noop } from "../interfaces.js"; import type { Subscriber } from "../observation/notifier.js"; import { ExecutionContext, @@ -411,6 +411,12 @@ export abstract class StatelessAttachedAttributeDirective */ public nodeId: string; + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; + /** * Creates an instance of RefDirective. * @param options - The options to use in configuring the directive. diff --git a/packages/web-components/fast-element/src/templating/node-observation.ts b/packages/web-components/fast-element/src/templating/node-observation.ts index a2405f6bd1f..00a3282883e 100644 --- a/packages/web-components/fast-element/src/templating/node-observation.ts +++ b/packages/web-components/fast-element/src/templating/node-observation.ts @@ -49,7 +49,7 @@ export const elements = (selector?: string): ElementsFilter => export abstract class NodeObservationDirective< T extends NodeBehaviorOptions > extends StatelessAttachedAttributeDirective { - private sourceProperty = `${this.id}-s`; + private controllerProperty = `${this.id}-c`; /** * Bind this behavior to the source. @@ -59,7 +59,7 @@ export abstract class NodeObservationDirective< */ bind(controller: ViewController): void { const target = controller.targets[this.nodeId] as any; - target[this.sourceProperty] = controller.source; + target[this.controllerProperty] = controller; this.updateTarget(controller.source, this.computeNodes(target)); this.observe(target); controller.onUnbind(this); @@ -75,7 +75,7 @@ export abstract class NodeObservationDirective< const target = controller.targets[this.nodeId] as any; this.updateTarget(controller.source, emptyArray); this.disconnect(target); - target[this.sourceProperty] = null; + target[this.controllerProperty] = null; } /** @@ -84,7 +84,7 @@ export abstract class NodeObservationDirective< * @returns The source. */ protected getSource(target: Node) { - return target[this.sourceProperty]; + return target[this.controllerProperty].source; } /** diff --git a/packages/web-components/fast-element/src/templating/ref.spec.ts b/packages/web-components/fast-element/src/templating/ref.spec.ts new file mode 100644 index 00000000000..c3dbac17043 --- /dev/null +++ b/packages/web-components/fast-element/src/templating/ref.spec.ts @@ -0,0 +1,38 @@ +import { expect } from "chai"; +import { ref } from "./ref.js"; +import { html } from "./template.js"; + +describe("the ref directive", () => { + class Model { + reference: HTMLDivElement; + } + + it("should capture an element reference", () => { + const template = html` +
+ `; + + const view = template.create(); + const model = new Model(); + + view.bind(model); + + expect(model.reference).instanceOf(HTMLDivElement); + expect(model.reference.id).equal("test"); + }); + + it("should not throw if DOM stringified", () => { + const template = html` +
+ `; + + const view = template.create(); + const model = new Model(); + + view.bind(model); + + expect(() => { + JSON.stringify(model.reference); + }).to.not.throw(); + }); +}); diff --git a/packages/web-components/fast-element/src/templating/slotted.spec.ts b/packages/web-components/fast-element/src/templating/slotted.spec.ts index 9f9a6d57f79..30256192db2 100644 --- a/packages/web-components/fast-element/src/templating/slotted.spec.ts +++ b/packages/web-components/fast-element/src/templating/slotted.spec.ts @@ -1,10 +1,12 @@ import { expect } from "chai"; import { slotted, SlottedDirective } from "./slotted.js"; +import { ref } from "./ref.js"; import { observable } from "../observation/observable.js"; import { elements } from "./node-observation.js"; import { Updates } from "../observation/update-queue.js"; import type { ViewBehaviorTargets, ViewController } from "./html-directive.js"; import { Fake } from "../testing/fakes.js"; +import { html } from "./template.js"; describe("The slotted", () => { context("template function", () => { @@ -28,6 +30,7 @@ describe("The slotted", () => { context("behavior", () => { class Model { @observable nodes; + reference: HTMLElement; } function createAndAppendChildren(host: HTMLElement, elementName = "div") { @@ -154,5 +157,25 @@ describe("The slotted", () => { expect(model.nodes).members([]); }); + + it("should not throw if DOM stringified", () => { + const template = html` + + + `; + + const view = template.create(); + const model = new Model(); + + view.bind(model); + + expect(() => { + JSON.stringify(model.reference); + }).to.not.throw(); + + view.unbind(); + }); }); }); diff --git a/packages/web-components/fast-element/src/templating/template.ts b/packages/web-components/fast-element/src/templating/template.ts index f77e8cb2e2e..ebfd71fe807 100644 --- a/packages/web-components/fast-element/src/templating/template.ts +++ b/packages/web-components/fast-element/src/templating/template.ts @@ -1,4 +1,4 @@ -import { isFunction, isString } from "../interfaces.js"; +import { isFunction, isString, noop } from "../interfaces.js"; import type { Expression } from "../observation/observable.js"; import { bind, HTMLBindingDirective, oneTime } from "./binding.js"; import { Compiler } from "./compiler.js"; @@ -124,6 +124,12 @@ export class ViewTemplate view.appendTo(host); return view; } + + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; } // Much thanks to LitHTML for working this out! diff --git a/packages/web-components/fast-element/src/templating/view.ts b/packages/web-components/fast-element/src/templating/view.ts index 7b020cb5501..5fa00a7d354 100644 --- a/packages/web-components/fast-element/src/templating/view.ts +++ b/packages/web-components/fast-element/src/templating/view.ts @@ -1,4 +1,4 @@ -import type { Disposable } from "../interfaces.js"; +import { Disposable, noop } from "../interfaces.js"; import { ExecutionContext, Observable, @@ -352,6 +352,12 @@ export class HTMLView this.isBound = false; } + /** + * Opts out of JSON stringification. + * @internal + */ + toJSON = noop; + private evaluateUnbindables() { const unbindables = this.unbindables; diff --git a/packages/web-components/fast-element/src/testing/fakes.ts b/packages/web-components/fast-element/src/testing/fakes.ts index ee15ffb8641..5e2edc889f7 100644 --- a/packages/web-components/fast-element/src/testing/fakes.ts +++ b/packages/web-components/fast-element/src/testing/fakes.ts @@ -4,6 +4,7 @@ import { ViewBehaviorTargets, ViewController, } from "../index.js"; +import { noop } from "../interfaces.js"; export const Fake = Object.freeze({ executionContext( @@ -108,6 +109,7 @@ export const Fake = Object.freeze({ }, source: (null as any) as TSource, targets, + toJSON: noop, bind( source: TSource, context: ExecutionContext = Fake.executionContext()