From 7c93499ccdfc493df9397163e552356e64bfd2c3 Mon Sep 17 00:00:00 2001 From: Justin Fagnani Date: Fri, 3 Feb 2023 18:00:05 -0800 Subject: [PATCH] Make ContextRoot deduplicate context requests by element and callback identity (#3451) --- .changeset/proud-beans-bake.md | 5 + packages/labs/context/README.md | 4 +- packages/labs/context/src/lib/context-root.ts | 119 ++++++++++------ .../src/lib/controllers/context-consumer.ts | 73 +++++----- .../context/src/test/late-provider_test.ts | 130 ++++++++++++++++-- packages/tests/src/web-test-runner.config.ts | 2 +- 6 files changed, 241 insertions(+), 92 deletions(-) create mode 100644 .changeset/proud-beans-bake.md diff --git a/.changeset/proud-beans-bake.md b/.changeset/proud-beans-bake.md new file mode 100644 index 0000000000..a232d40d31 --- /dev/null +++ b/.changeset/proud-beans-bake.md @@ -0,0 +1,5 @@ +--- +'@lit-labs/context': patch +--- + +Make ContextRoot deduplicate context requests by element and callback identity diff --git a/packages/labs/context/README.md b/packages/labs/context/README.md index c7dba071c8..625a2e2119 100644 --- a/packages/labs/context/README.md +++ b/packages/labs/context/README.md @@ -153,7 +153,9 @@ root.attach(document.body); The `ContextRoot` can be attached to any element and it will gather a list of any context requests which are received at the attached element. The `ContextProvider` controllers will emit `context-provider` events when they are connected to the DOM. These events act as triggers for the `ContextRoot` to redispatch these `context-request` events from their sources. -This solution has a small overhead, in that if a provider is not within the DOM hierarchy of the unsatisfied requests we are unnecessarily refiring these requests, but this approach is safest and most correct in that it is very hard to manage stable DOM hierarchies with the semantics of slotting and reparenting that is common in web components implementations. +This solution has a small overhead, in that if a provider is not within the DOM hierarchy of the unsatisfied requests we are unnecessarily refiring these requests, but this approach is safest and most correct in that it is very hard to manage unstable DOM hierarchies with the semantics of slotting and reparenting that is common in web components implementations. + +Note that ContextRoot uses [WeakRefs](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/WeakRef) which are not supported in IE11. ## Contributing diff --git a/packages/labs/context/src/lib/context-root.ts b/packages/labs/context/src/lib/context-root.ts index acb428e58d..df4c59af25 100644 --- a/packages/labs/context/src/lib/context-root.ts +++ b/packages/labs/context/src/lib/context-root.ts @@ -5,27 +5,31 @@ */ import {Context} from './create-context.js'; -import {ContextRequest, ContextRequestEvent} from './context-request-event.js'; +import {ContextCallback, ContextRequestEvent} from './context-request-event.js'; import {ContextProviderEvent} from './controllers/context-provider.js'; -type UnknownContextKey = Context; - /** - * A context request, with associated source element, with all objects as weak references. - */ -type PendingContextRequest = Omit< - ContextRequest, - 'context' | 'subscribe' -> & {element: HTMLElement}; - -/** - * A ContextRoot can be used to gather unsatisfied context requests and redispatch these - * requests when new providers which satisfy matching context keys are available. + * A ContextRoot buffers unsatisfied context request events. It will redispatch + * these requests when new providers which satisfy matching contexts + * are available. */ export class ContextRoot { private pendingContextRequests = new Map< - UnknownContextKey, - Set + Context, + { + // The WeakMap lets us detect if we're seen an element/callback pair yet, + // without needing to iterate the `requests` array + callbacks: WeakMap>>; + + // Requests lets us iterate over every element/callback that we need to + // replay context events for + // Both the element and callback must be stored in WeakRefs because the + // callback most likely has a strong ref to the element. + requests: Array<{ + elementRef: WeakRef; + callbackRef: WeakRef>; + }>; + } >(); /** @@ -50,49 +54,74 @@ export class ContextRoot { } private onContextProvider = ( - ev: ContextProviderEvent> + event: ContextProviderEvent> ) => { - const pendingRequests = this.pendingContextRequests.get(ev.context); - if (!pendingRequests) { - return; // no pending requests for this provider at this time + const pendingRequestData = this.pendingContextRequests.get(event.context); + if (pendingRequestData === undefined) { + // No pending requests for this context at this time + return; } - // clear our list, any still unsatisfied requests will re-add themselves - this.pendingContextRequests.delete(ev.context); + // Clear our list. Any still unsatisfied requests will re-add themselves + // when we dispatch the events below. + this.pendingContextRequests.delete(event.context); + + // Loop over all pending requests and re-dispatch them from their source + const {requests} = pendingRequestData; + for (const {elementRef, callbackRef} of requests) { + const element = elementRef.deref(); + const callback = callbackRef.deref(); - // loop over all pending requests and re-dispatch them from their source - pendingRequests.forEach((request) => { - const element = request.element; - const callback = request.callback; - // redispatch if we still have all the parts of the request - if (element) { + if (element === undefined || callback === undefined) { + // The element was GC'ed. Do nothing. + } else { + // Re-dispatch if we still have the element and callback element.dispatchEvent( - new ContextRequestEvent(ev.context, callback, true) + new ContextRequestEvent(event.context, callback, true) ); } - }); + } }; private onContextRequest = ( - ev: ContextRequestEvent> + event: ContextRequestEvent> ) => { - // events that are not subscribing should not be captured - if (!ev.subscribe) { + // Events that are not subscribing should not be buffered + if (event.subscribe !== true) { return; } - // store a weakref to this element under the context key - const request: PendingContextRequest = { - element: ev.target as HTMLElement, - callback: ev.callback, - }; - let pendingContextRequests = this.pendingContextRequests.get(ev.context); - if (!pendingContextRequests) { - pendingContextRequests = new Set(); - this.pendingContextRequests.set(ev.context, pendingContextRequests); + + const element = event.target as HTMLElement; + const callback = event.callback; + + let pendingContextRequests = this.pendingContextRequests.get(event.context); + if (pendingContextRequests === undefined) { + this.pendingContextRequests.set( + event.context, + (pendingContextRequests = { + callbacks: new WeakMap(), + requests: [], + }) + ); } - // NOTE: if the element is connected multiple times it will add itself - // to this set multiple times since the set identify of the request - // object will be unique each time. - pendingContextRequests.add(request); + + let callbacks = pendingContextRequests.callbacks.get(element); + if (callbacks === undefined) { + pendingContextRequests.callbacks.set( + element, + (callbacks = new WeakSet()) + ); + } + + if (callbacks.has(callback)) { + // We're already tracking this element/callback pair + return; + } + + callbacks.add(callback); + pendingContextRequests.requests.push({ + elementRef: new WeakRef(element), + callbackRef: new WeakRef(callback), + }); }; } diff --git a/packages/labs/context/src/lib/controllers/context-consumer.ts b/packages/labs/context/src/lib/controllers/context-consumer.ts index 9775b53c83..6f73f4c381 100644 --- a/packages/labs/context/src/lib/controllers/context-consumer.ts +++ b/packages/labs/context/src/lib/controllers/context-consumer.ts @@ -4,7 +4,10 @@ * SPDX-License-Identifier: BSD-3-Clause */ -import {ContextRequestEvent} from '../context-request-event.js'; +import { + ContextCallback, + ContextRequestEvent, +} from '../context-request-event.js'; import {Context, ContextType} from '../create-context.js'; import {ReactiveController, ReactiveElement} from 'lit'; @@ -48,41 +51,41 @@ export class ContextConsumer< private dispatchRequest() { this.host.dispatchEvent( - new ContextRequestEvent( - this.context, - (value, unsubscribe) => { - // some providers will pass an unsubscribe function indicating they may provide future values - if (this.unsubscribe) { - // if the unsubscribe function changes this implies we have changed provider - if (this.unsubscribe !== unsubscribe) { - // cleanup the old provider - this.provided = false; - this.unsubscribe(); - } - // if we don't support subscription, immediately unsubscribe - if (!this.subscribe) { - this.unsubscribe(); - } - } + new ContextRequestEvent(this.context, this._callback, this.subscribe) + ); + } + + // This function must have stable identity to properly dedupe in ContextRoot + // if this element connects multiple times. + private _callback: ContextCallback> = (value, unsubscribe) => { + // some providers will pass an unsubscribe function indicating they may provide future values + if (this.unsubscribe) { + // if the unsubscribe function changes this implies we have changed provider + if (this.unsubscribe !== unsubscribe) { + // cleanup the old provider + this.provided = false; + this.unsubscribe(); + } + // if we don't support subscription, immediately unsubscribe + if (!this.subscribe) { + this.unsubscribe(); + } + } - // store the value so that it can be retrieved from the controller - this.value = value; - // schedule an update in case this value is used in a template - this.host.requestUpdate(); + // store the value so that it can be retrieved from the controller + this.value = value; + // schedule an update in case this value is used in a template + this.host.requestUpdate(); - // only invoke callback if we are either expecting updates or have not yet - // been provided a value - if (!this.provided || this.subscribe) { - this.provided = true; - if (this.callback) { - this.callback(value, unsubscribe); - } - } + // only invoke callback if we are either expecting updates or have not yet + // been provided a value + if (!this.provided || this.subscribe) { + this.provided = true; + if (this.callback) { + this.callback(value, unsubscribe); + } + } - this.unsubscribe = unsubscribe; - }, - this.subscribe - ) - ); - } + this.unsubscribe = unsubscribe; + }; } diff --git a/packages/labs/context/src/test/late-provider_test.ts b/packages/labs/context/src/test/late-provider_test.ts index ff40e35d53..d76a8a9f74 100644 --- a/packages/labs/context/src/test/late-provider_test.ts +++ b/packages/labs/context/src/test/late-provider_test.ts @@ -13,6 +13,7 @@ import { provide, ContextRoot, ContextProvider, + ContextConsumer, } from '@lit-labs/context'; import {assert} from '@esm-bundle/chai'; @@ -47,14 +48,13 @@ class LateContextProviderElement extends LitElement { } } -@customElement('lazy-context-provider') -export class LazyContextProviderElement extends LitElement { - protected render() { - return html``; - } -} +const ua = window.navigator.userAgent; +const isIE = ua.indexOf('Trident/') > 0; + +const suiteSkipIE: typeof suite.skip = (...args) => + isIE ? suite.skip(...args) : suite(...args); -suite('late context provider', () => { +suiteSkipIE('late context provider', () => { // let consumer: ContextConsumerElement; // let provider: LateContextProviderElement; let container: HTMLElement; @@ -114,10 +114,16 @@ suite('late context provider', () => { }); test('lazy added provider', async () => { + @customElement('lazy-context-provider') + class LazyContextProviderElement extends LitElement { + protected render() { + return html``; + } + } container.innerHTML = ` - - - + + + `; const provider = container.querySelector( @@ -139,4 +145,108 @@ suite('late context provider', () => { // `value` should now be provided assert.strictEqual(consumer.value, 1000); }); + + test('late element with multiple properties', async () => { + @customElement('context-consumer-2') + class ContextConsumer2Element extends LitElement { + @consume({context: simpleContext, subscribe: true}) + @property({type: Number}) + public value1 = 0; + + @consume({context: simpleContext, subscribe: true}) + @property({type: Number}) + public value2 = 0; + } + + container.innerHTML = ` + + + + `; + + const provider = container.querySelector( + 'late-context-provider-2' + ) as LateContextProviderElement; + + const consumer = container.querySelector( + 'context-consumer-2' + ) as ContextConsumer2Element; + + // Let consumer update once with no provider + await consumer.updateComplete; + + // Define provider element + customElements.define( + 'late-context-provider-2', + class extends LateContextProviderElement {} + ); + + await provider.updateComplete; + await consumer.updateComplete; + + // Check that regardless of de-duping in ContextRoot, both @consume() + // decorated properties were set. + assert.equal(consumer.value1, 999, 'value1'); + assert.equal(consumer.value2, 999, 'value2'); + }); + + test('a moved component is only provided to once', async () => { + @customElement('context-consumer-3') + class ContextConsumer3Element extends LitElement { + _consume = new ContextConsumer( + this, + simpleContext, + (value) => { + this.value = value; + this.callCount++; + }, + true + ); + + value: number | undefined = undefined; + + callCount = 0; + } + + container.innerHTML = ` + +
+ +
+
+ + `; + + const provider = container.querySelector( + 'late-context-provider-3' + ) as LateContextProviderElement; + + const consumer = container.querySelector( + 'context-consumer-3' + ) as ContextConsumer3Element; + + const parent2 = container.querySelector('#parent-2')!; + + // Let consumer update once with no provider + await consumer.updateComplete; + + // Re-parent the consumer so it dispatches a new context-request event + parent2.append(consumer); + + // Let consumer update again with no provider + await consumer.updateComplete; + + // Define provider element + customElements.define( + 'late-context-provider-3', + class extends LateContextProviderElement {} + ); + + await provider.updateComplete; + await consumer.updateComplete; + + assert.equal(consumer.value, 999); + // Check that the consumer was called only once + assert.equal(consumer.callCount, 1); + }); }); diff --git a/packages/tests/src/web-test-runner.config.ts b/packages/tests/src/web-test-runner.config.ts index 7ca50297b0..65822880a3 100644 --- a/packages/tests/src/web-test-runner.config.ts +++ b/packages/tests/src/web-test-runner.config.ts @@ -38,7 +38,7 @@ const browserPresets = { sauce: [ 'sauce:Windows 10/Firefox@91', // Current ESR. See: https://wiki.mozilla.org/Release_Management/Calendar 'sauce:Windows 10/Chrome@latest-2', - 'sauce:macOS 10.15/Safari@latest', + 'sauce:macOS 11/Safari@latest', // 'sauce:Windows 10/MicrosoftEdge@18', // needs globalThis polyfill ], 'sauce-ie11': ['sauce:Windows 10/Internet Explorer@11'],