From 85f8b628b5d3806c9d534aa12c0abf29ff976622 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Tue, 9 Jul 2024 12:35:22 +0300 Subject: [PATCH 1/8] feat(core): add `@listen` decorator --- .changeset/fluffy-papers-sit.md | 16 ++++++++++++++++ core/pfe-core/decorators.ts | 1 + core/pfe-core/decorators/listen.ts | 26 ++++++++++++++++++++++++++ 3 files changed, 43 insertions(+) create mode 100644 .changeset/fluffy-papers-sit.md create mode 100644 core/pfe-core/decorators/listen.ts diff --git a/.changeset/fluffy-papers-sit.md b/.changeset/fluffy-papers-sit.md new file mode 100644 index 0000000000..8659948613 --- /dev/null +++ b/.changeset/fluffy-papers-sit.md @@ -0,0 +1,16 @@ +--- +"@patternfly/pfe-core": minor +--- +**Decorators**: Added `@listen`. Use it to attach element event listeners to +class methods. + +```ts +@customElement('custom-input') +class CustomInput extends LitElement { + @property({ type: Boolean }) dirty = false; + @listen('keyup', { once: true }) + protected onKeyup() { + this.dirty = true; + } +} +``` diff --git a/core/pfe-core/decorators.ts b/core/pfe-core/decorators.ts index 18ffd86b6e..8a0c6aa2ff 100644 --- a/core/pfe-core/decorators.ts +++ b/core/pfe-core/decorators.ts @@ -2,6 +2,7 @@ export * from './decorators/bound.js'; export * from './decorators/cascades.js'; export * from './decorators/deprecation.js'; export * from './decorators/initializer.js'; +export * from './decorators/listen.js'; export * from './decorators/observed.js'; export * from './decorators/time.js'; export * from './decorators/trace.js'; diff --git a/core/pfe-core/decorators/listen.ts b/core/pfe-core/decorators/listen.ts new file mode 100644 index 0000000000..9c176cd58e --- /dev/null +++ b/core/pfe-core/decorators/listen.ts @@ -0,0 +1,26 @@ +import type { LitElement } from 'lit'; + +/** + * Listens for a given event on the custom element. + * equivalent to calling `this.addEventListener` in the constructor + * @param type event type e.g. `click` + * @param options event listener options object e.g. `{ passive: true }` + */ +export function listen(type: string, options?: EventListenerOptions) { + return function( + proto: LitElement, + methodName: string, + ) { + const origConnected = proto.connectedCallback; + const origDisconnected = proto.disconnectedCallback; + const listener = (proto as any)[methodName] as EventListener; + proto.connectedCallback = function() { + origConnected(); + this.addEventListener(type, listener, options); + }; + proto.disconnectedCallback = function() { + origDisconnected(); + this.removeEventListener(type, listener, options); + }; + }; +} From 5ec3e011a06f89a2a940794e9ed01bc3030a428b Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Tue, 9 Jul 2024 12:35:45 +0300 Subject: [PATCH 2/8] feat(core): add `@observes` decorator --- .changeset/poor-years-hug.md | 21 +++++++++++++++++++ core/pfe-core/decorators.ts | 1 + core/pfe-core/decorators/observes.ts | 30 ++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+) create mode 100644 .changeset/poor-years-hug.md create mode 100644 core/pfe-core/decorators/observes.ts diff --git a/.changeset/poor-years-hug.md b/.changeset/poor-years-hug.md new file mode 100644 index 0000000000..0145dc20af --- /dev/null +++ b/.changeset/poor-years-hug.md @@ -0,0 +1,21 @@ +--- +"@patternfly/pfe-core": minor +--- +**Decorators**: Added `@observes`. Use it to add property change callback by +decorating them with the name of the property to observe + +```ts +@customElement('custom-button') +class CustomButton extends LitElement { + #internals = this.attachInternals(); + + @property({ type: Boolean }) disabled = false; + + @observes('disabled') + protected disabledChanged() { + this.#internals.ariaDisabled = + this.disabled ? 'true' + : this.ariaDisabled ?? 'false'; + } +} +``` diff --git a/core/pfe-core/decorators.ts b/core/pfe-core/decorators.ts index 8a0c6aa2ff..17bdfe6736 100644 --- a/core/pfe-core/decorators.ts +++ b/core/pfe-core/decorators.ts @@ -4,5 +4,6 @@ export * from './decorators/deprecation.js'; export * from './decorators/initializer.js'; export * from './decorators/listen.js'; export * from './decorators/observed.js'; +export * from './decorators/observes.js'; export * from './decorators/time.js'; export * from './decorators/trace.js'; diff --git a/core/pfe-core/decorators/observes.ts b/core/pfe-core/decorators/observes.ts new file mode 100644 index 0000000000..26b854f52c --- /dev/null +++ b/core/pfe-core/decorators/observes.ts @@ -0,0 +1,30 @@ +import type { ReactiveElement } from 'lit'; + +import type { ChangeCallback } from '@patternfly/pfe-core/controllers/property-observer-controller.js'; + +/** + * Observes changes on the given property and calls the decorated method + * with the old and new values when it changes. + * @param propertyName property to react to + */ +export function observes(propertyName: string & keyof T) { + return function(proto: T, methodName: string) { + const method = proto[methodName as keyof T] as ChangeCallback; + if (typeof method !== 'function') { + throw new Error('@observes must decorate a class method'); + } + const descriptor = Object.getOwnPropertyDescriptor(proto, propertyName); + Object.defineProperty(proto, propertyName, { + ...descriptor, + configurable: true, + set(this: T, newVal: T[keyof T]) { + const oldVal = this[propertyName as keyof T]; + // first, call any pre-existing setters, e.g. `@property` + descriptor?.set?.call(this, newVal); + method.call(this, oldVal, newVal); + }, + }); + }; +} + + From 13d7511deeb8768ea941474131fdbf4fdfb1d0f5 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Tue, 9 Jul 2024 12:37:12 +0300 Subject: [PATCH 3/8] refactor: use new listen and observes decorators --- elements/pf-accordion/pf-accordion.ts | 20 ++++++++++++-------- elements/pf-jump-links/pf-jump-links-item.ts | 19 +++++++++---------- elements/pf-modal/pf-modal.ts | 12 ++++++------ elements/pf-tabs/pf-tab.ts | 10 +++++----- 4 files changed, 32 insertions(+), 29 deletions(-) diff --git a/elements/pf-accordion/pf-accordion.ts b/elements/pf-accordion/pf-accordion.ts index 729f17533c..d75ffea42b 100644 --- a/elements/pf-accordion/pf-accordion.ts +++ b/elements/pf-accordion/pf-accordion.ts @@ -1,5 +1,5 @@ import { LitElement, html } from 'lit'; -import { observed } from '@patternfly/pfe-core/decorators.js'; +import { listen, observes } from '@patternfly/pfe-core/decorators.js'; import { property } from 'lit/decorators/property.js'; import { customElement } from 'lit/decorators/custom-element.js'; @@ -114,9 +114,6 @@ export class PfAccordion extends LitElement { @property({ type: Boolean, reflect: true }) bordered = false; /** Whether to apply the `large` style variant */ - @observed(function largeChanged(this: PfAccordion) { - [...this.headers, ...this.panels].forEach(el => el.toggleAttribute('large', this.large)); - }) @property({ type: Boolean, reflect: true }) large = false; @property({ type: Boolean, reflect: true }) fixed = false; @@ -183,7 +180,6 @@ export class PfAccordion extends LitElement { connectedCallback() { super.connectedCallback(); - this.addEventListener('change', this.#onChange as EventListener); this.#mo.observe(this, { childList: true }); this.#init(); } @@ -221,6 +217,13 @@ export class PfAccordion extends LitElement { return c && results.every(Boolean); } + @observes('large') + protected largeChanged() { + for (const el of [...this.headers, ...this.panels]) { + el.toggleAttribute('large', this.large); + } + } + /** * Initialize the accordion by connecting headers and panels * with aria controls and labels; set up the default disclosure @@ -280,7 +283,8 @@ export class PfAccordion extends LitElement { panel.hidden = true; } - #onChange(event: PfAccordionHeaderChangeEvent) { + @listen('change') + protected onChange(event: PfAccordionHeaderChangeEvent) { if (event instanceof PfAccordionHeaderChangeEvent && event.accordion === this) { const index = this.#getIndex(event.target); if (event.expanded) { @@ -358,8 +362,8 @@ export class PfAccordion extends LitElement { } // If the header and panel exist, open both - this.#expandHeader(header, index), - this.#expandPanel(panel), + this.#expandHeader(header, index); + this.#expandPanel(panel); header.focus(); diff --git a/elements/pf-jump-links/pf-jump-links-item.ts b/elements/pf-jump-links/pf-jump-links-item.ts index 867612fb89..e1d3c85c57 100644 --- a/elements/pf-jump-links/pf-jump-links-item.ts +++ b/elements/pf-jump-links/pf-jump-links-item.ts @@ -8,7 +8,7 @@ import { InternalsController } from '@patternfly/pfe-core/controllers/internals- import style from './pf-jump-links-item.css'; -import { observed } from '@patternfly/pfe-core/decorators/observed.js'; +import { observes } from '@patternfly/pfe-core/decorators/observes.js'; /** * @cssprop --pf-c-jump-links__link--PaddingTop -- padding around the link @@ -28,29 +28,28 @@ export class PfJumpLinksItem extends LitElement { }; /** Whether this item is active. */ - @observed('activeChanged') @property({ type: Boolean, reflect: true }) active = false; /** hypertext reference for this link */ @property({ reflect: true }) href?: string; - #internals = InternalsController.of(this, { role: 'listitem' }); - - override connectedCallback() { - super.connectedCallback(); - this.activeChanged(); - } + #internals = InternalsController.of(this, { + role: 'listitem', + }); render() { return html` - + `; } - private activeChanged() { + @observes('active') + protected activeChanged() { this.#internals.ariaCurrent = this.active ? 'location' : null; } diff --git a/elements/pf-modal/pf-modal.ts b/elements/pf-modal/pf-modal.ts index 866a9c95ef..53d13e9f7a 100644 --- a/elements/pf-modal/pf-modal.ts +++ b/elements/pf-modal/pf-modal.ts @@ -6,7 +6,7 @@ import { ifDefined } from 'lit/directives/if-defined.js'; import { classMap } from 'lit/directives/class-map.js'; import { ComposedEvent } from '@patternfly/pfe-core'; -import { bound, initializer, observed } from '@patternfly/pfe-core/decorators.js'; +import { bound, initializer, observes } from '@patternfly/pfe-core/decorators.js'; import { getRandomId } from '@patternfly/pfe-core/functions/random.js'; import { SlotController } from '@patternfly/pfe-core/controllers/slot-controller.js'; @@ -67,7 +67,7 @@ export class ModalOpenEvent extends ComposedEvent { */ @customElement('pf-modal') export class PfModal extends LitElement implements HTMLDialogElement { - static override readonly shadowRootOptions = { + static override readonly shadowRootOptions: ShadowRootInit = { ...LitElement.shadowRootOptions, delegatesFocus: true, }; @@ -88,11 +88,9 @@ export class PfModal extends LitElement implements HTMLDialogElement { */ @property({ reflect: true }) position?: 'top'; - @observed @property({ type: Boolean, reflect: true }) open = false; /** Optional ID of the trigger element */ - @observed @property() trigger?: string; /** @see https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement/returnValue */ @@ -190,7 +188,8 @@ export class PfModal extends LitElement implements HTMLDialogElement { } } - protected async _openChanged(oldValue?: boolean, newValue?: boolean) { + @observes('open') + protected async openChanged(oldValue?: boolean, newValue?: boolean) { // loosening types to prevent running these effects in unexpected circumstances // eslint-disable-next-line eqeqeq if (oldValue == null || newValue == null || oldValue == newValue) { @@ -216,7 +215,8 @@ export class PfModal extends LitElement implements HTMLDialogElement { } } - protected _triggerChanged() { + @observes('trigger') + protected triggerChanged() { if (this.trigger) { this.#triggerElement = (this.getRootNode() as Document | ShadowRoot) .getElementById(this.trigger); diff --git a/elements/pf-tabs/pf-tab.ts b/elements/pf-tabs/pf-tab.ts index 3cc3ff9977..8533bd1b34 100644 --- a/elements/pf-tabs/pf-tab.ts +++ b/elements/pf-tabs/pf-tab.ts @@ -5,7 +5,7 @@ import { queryAssignedElements } from 'lit/decorators/query-assigned-elements.js import { classMap } from 'lit/directives/class-map.js'; import { consume } from '@lit/context'; -import { observed } from '@patternfly/pfe-core/decorators.js'; +import { observes } from '@patternfly/pfe-core/decorators.js'; import { getRandomId } from '@patternfly/pfe-core/functions/random.js'; import { InternalsController } from '@patternfly/pfe-core/controllers/internals-controller.js'; @@ -65,10 +65,8 @@ export class PfTab extends LitElement { @queryAssignedElements({ slot: 'icon', flatten: true }) private icons!: HTMLElement[]; - @observed @property({ reflect: true, type: Boolean }) active = false; - @observed @property({ reflect: true, type: Boolean }) disabled = false; @consume({ context, subscribe: true }) @@ -144,14 +142,16 @@ export class PfTab extends LitElement { return this.dispatchEvent(new TabExpandEvent(this)); } - private _activeChanged(old: boolean) { + @observes('active') + protected activeChanged(old: boolean) { this.#internals.ariaSelected = String(!!this.active); if (this.active && !old) { this.#activate(); } } - private _disabledChanged() { + @observes('disabled') + protected disabledChanged() { this.#internals.ariaDisabled = this.disabled ? 'true' : this.ariaDisabled ?? 'false'; } } From fdcd4aaefc339aa2953d0f0d2a944dfca0efcc6e Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Wed, 10 Jul 2024 08:56:26 +0300 Subject: [PATCH 4/8] fix(core): listen decorator call lifecycle correctly --- core/pfe-core/decorators/listen.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/pfe-core/decorators/listen.ts b/core/pfe-core/decorators/listen.ts index 9c176cd58e..a59faf720a 100644 --- a/core/pfe-core/decorators/listen.ts +++ b/core/pfe-core/decorators/listen.ts @@ -10,16 +10,16 @@ export function listen(type: string, options?: EventListenerOptions) { return function( proto: LitElement, methodName: string, - ) { + ): void { const origConnected = proto.connectedCallback; const origDisconnected = proto.disconnectedCallback; const listener = (proto as any)[methodName] as EventListener; proto.connectedCallback = function() { - origConnected(); + origConnected?.call(this); this.addEventListener(type, listener, options); }; proto.disconnectedCallback = function() { - origDisconnected(); + origDisconnected?.call(this); this.removeEventListener(type, listener, options); }; }; From 3b977a38675499d935e6683389aa2b2ae08a61ed Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Wed, 10 Jul 2024 08:59:29 +0300 Subject: [PATCH 5/8] test(core): observes decorator test --- core/pfe-core/test/decorators.spec.ts | 38 +++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 core/pfe-core/test/decorators.spec.ts diff --git a/core/pfe-core/test/decorators.spec.ts b/core/pfe-core/test/decorators.spec.ts new file mode 100644 index 0000000000..c459043bbc --- /dev/null +++ b/core/pfe-core/test/decorators.spec.ts @@ -0,0 +1,38 @@ +import { LitElement, html } from 'lit'; +import { fixture, expect } from '@open-wc/testing'; +import { customElement } from 'lit/decorators/custom-element.js'; +import { property } from 'lit/decorators/property.js'; +import { observes } from '../decorators/observes.js'; + +@customElement('x-observes-host') +class XObservesHost extends LitElement { + @property({ type: Number }) count = 0; + + old?: number; + current?: number; + + @observes('count') numberChanged(old: number, current: number) { + this.old = old; + this.current = current; + } +} + +describe('@observes', function() { + let element: XObservesHost; + beforeEach(async function() { + element = await fixture(html``); + }); + it('initializes with old and new values', function() { + expect(element.old).to.equal(undefined); + expect(element.current).to.equal(0); + }); + describe('setting the observed prop', function() { + beforeEach(function() { + element.count++; + }); + it('updates old and new values', function() { + expect(element.old).to.equal(0); + expect(element.current).to.equal(1); + }); + }); +}); From 225a41c105fdd41787cc15e1374cdd896b41c0e1 Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Wed, 10 Jul 2024 09:00:14 +0300 Subject: [PATCH 6/8] feat(core): observes decorator options waitFor option lets caller wait for connected, firstUpdated, or updated until calling the method --- .changeset/poor-years-hug.md | 9 +- .../property-observer-controller.ts | 85 +++++++------ core/pfe-core/decorators/observed.ts | 111 ++++++++--------- core/pfe-core/decorators/observes.ts | 44 ++++--- core/pfe-core/test/decorators.spec.ts | 114 ++++++++++++++++-- 5 files changed, 240 insertions(+), 123 deletions(-) diff --git a/.changeset/poor-years-hug.md b/.changeset/poor-years-hug.md index 0145dc20af..195b11abf9 100644 --- a/.changeset/poor-years-hug.md +++ b/.changeset/poor-years-hug.md @@ -1,5 +1,5 @@ --- -"@patternfly/pfe-core": minor +"@patternfly/pfe-core": major --- **Decorators**: Added `@observes`. Use it to add property change callback by decorating them with the name of the property to observe @@ -19,3 +19,10 @@ class CustomButton extends LitElement { } } ``` + +Breaking change: This commit makes some changes to the internal APIs of the +pre-existing `@observed` observer, most notably it changes the constructor +signature of the `PropertyObserverController` and associated functions. Most +users should not have to make any changes, but if you directly import and use +those functions, check the commit log to see how to update your call sites. + diff --git a/core/pfe-core/controllers/property-observer-controller.ts b/core/pfe-core/controllers/property-observer-controller.ts index 0703f8efb1..faac228bec 100644 --- a/core/pfe-core/controllers/property-observer-controller.ts +++ b/core/pfe-core/controllers/property-observer-controller.ts @@ -1,52 +1,59 @@ import type { ReactiveController, ReactiveElement } from 'lit'; -export const observedController: unique symbol = Symbol('observed properties controller'); - -export type ChangeCallback = ( +export type ChangeCallback = ( this: T, - old?: T[keyof T], - newV?: T[keyof T], + old?: V, + newV?: V, ) => void; -export type ChangeCallbackName = `_${string}Changed`; - -export type PropertyObserverHost = T & Record> & { - [observedController]: PropertyObserverController; -}; - -/** This controller holds a cache of observed property values which were set before the element updated */ -export class PropertyObserverController implements ReactiveController { - private static hosts = new WeakMap(); - - private values = new Map(); +export interface PropertyObserverOptions { + propertyName: string & keyof T; + callback: ChangeCallback; + waitFor?: 'connected' | 'updated' | 'firstUpdated'; +} - private delete(key: string) { - this.values.delete(key); - } +export class PropertyObserverController< + T extends ReactiveElement +> implements ReactiveController { + private oldVal: T[keyof T]; - constructor(private host: ReactiveElement) { - if (PropertyObserverController.hosts.get(host)) { - return PropertyObserverController.hosts.get(host) as PropertyObserverController; - } - host.addController(this); - (host as PropertyObserverHost)[observedController] = this; + constructor( + private host: T, + private options: PropertyObserverOptions + ) { + this.oldVal = host[options.propertyName]; } /** Set any cached valued accumulated between constructor and connectedCallback */ - hostUpdate(): void { - for (const [key, [methodName, [oldVal, newVal]]] of this.values) { - // @ts-expect-error: be cool, typescript - this.host[methodName as keyof ReactiveElement]?.(oldVal, newVal); - this.delete(key); + async hostUpdate(): Promise { + const { oldVal, options: { waitFor, propertyName, callback } } = this; + if (!callback) { + throw new Error(`no callback for ${propertyName}`); } - } - - /** Once the element has updated, we no longer need this controller, so we remove it */ - hostUpdated(): void { - this.host.removeController(this); - } - - cache(key: string, methodName: string, ...vals: [unknown, unknown]): void { - this.values.set(key, [methodName, vals]); + const newVal = this.host[propertyName]; + this.oldVal = newVal; + if (newVal !== oldVal) { + switch (waitFor) { + case 'connected': + if (!this.host.isConnected) { + const origConnected = this.host.connectedCallback; + await new Promise(resolve => { + this.host.connectedCallback = function() { + resolve(origConnected?.call(this)); + }; + }); + } + break; + case 'firstUpdated': + if (!this.host.hasUpdated) { + await this.host.updateComplete; + } + break; + case 'updated': + await this.host.updateComplete; + break; + } + } + callback.call(this.host, oldVal as T[keyof T], newVal); } } diff --git a/core/pfe-core/decorators/observed.ts b/core/pfe-core/decorators/observed.ts index d0096b5662..13c39d4d9b 100644 --- a/core/pfe-core/decorators/observed.ts +++ b/core/pfe-core/decorators/observed.ts @@ -1,14 +1,7 @@ import type { ReactiveElement } from 'lit'; -import type { - ChangeCallback, - ChangeCallbackName, - PropertyObserverHost, -} from '../controllers/property-observer-controller.js'; +import type { ChangeCallback } from '../controllers/property-observer-controller.js'; -import { - observedController, - PropertyObserverController, -} from '../controllers/property-observer-controller.js'; +import { PropertyObserverController } from '../controllers/property-observer-controller.js'; type TypedFieldDecorator = (proto: T, key: string | keyof T) => void ; @@ -35,66 +28,68 @@ type TypedFieldDecorator = (proto: T, key: string | keyof T) => void ; * @observed((oldVal, newVal) => console.log(`Size changed from ${oldVal} to ${newVal}`)) * ``` */ +export function observed( + cb: ChangeCallback, +): TypedFieldDecorator; export function observed(methodName: string): TypedFieldDecorator; -export function observed(cb: ChangeCallback): TypedFieldDecorator; export function observed(proto: T, key: string): void; // eslint-disable-next-line jsdoc/require-jsdoc export function observed(...as: any[]): void | TypedFieldDecorator { if (as.length === 1) { - const [methodNameOrCallback] = as; - return function(proto, key) { - (proto.constructor as typeof ReactiveElement) - .addInitializer(x => new PropertyObserverController(x)); - observeProperty(proto, key as string & keyof T, methodNameOrCallback); - }; + const [methodNameOrCb] = as; + return configuredDecorator(methodNameOrCb); } else { - const [proto, key] = as; - (proto.constructor as typeof ReactiveElement) - .addInitializer(x => new PropertyObserverController(x)); - observeProperty(proto, key); + return executeBareDecorator(...as as [T, string & keyof T]); } } /** - * Creates an observer on a field - * @param proto - * @param key - * @param callbackOrMethod + * @param proto element prototype + * @param propertyName propertyName + * @example ```typescript + * @observed @property() foo?: string; + * ``` */ -export function observeProperty( - proto: T, - key: string & keyof T, - callbackOrMethod?: ChangeCallback -): void { - const descriptor = Object.getOwnPropertyDescriptor(proto, key); - Object.defineProperty(proto, key, { - ...descriptor, - configurable: true, - set(this: PropertyObserverHost, newVal: T[keyof T]) { - const oldVal = this[key as keyof T]; - // first, call any pre-existing setters, e.g. `@property` - descriptor?.set?.call(this, newVal); +function executeBareDecorator(proto: T, propertyName: string & keyof T) { + const klass = proto.constructor as typeof ReactiveElement; + klass.addInitializer(x => initialize( + x as T, + propertyName, + x[`_${propertyName}Changed` as keyof typeof x] as ChangeCallback, + )); +} - // if the user passed a callback, call it - // e.g. `@observed((_, newVal) => console.log(newVal))` - // safe to call before connectedCallback, because it's impossible to get a `this` ref. - if (typeof callbackOrMethod === 'function') { - callbackOrMethod.call(this, oldVal, newVal); - } else { - // if the user passed a string method name, call it on `this` - // e.g. `@observed('_renderOptions')` - // otherwise, use a default method name e.g. `_fooChanged` - const actualMethodName = callbackOrMethod || `_${key}Changed`; +/** + * @param methodNameOrCb string name of callback or function + * @example ```typescript + * @observed('_myCallback') @property() foo?: string; + * @observed((old) => console.log(old)) @property() bar?: string; + * ``` + */ +function configuredDecorator( + methodNameOrCb: string | ChangeCallback, +): TypedFieldDecorator { + return function(proto, key) { + const propertyName = key as string & keyof T; + const klass = proto.constructor as typeof ReactiveElement; + if (typeof methodNameOrCb === 'function') { + const callback = methodNameOrCb; + klass.addInitializer(x => initialize(x as T, propertyName, callback)); + } else { + klass.addInitializer(x => initialize( + x as T, + propertyName, + x[methodNameOrCb as keyof ReactiveElement] as ChangeCallback, + )); + } + }; +} - // if the component has already connected to the DOM, run the callback - // otherwise, If the component has not yet connected to the DOM, - // cache the old and new values. See PropertyObserverController above - if (this.hasUpdated) { - this[actualMethodName as ChangeCallbackName]?.(oldVal, newVal); - } else { - this[observedController].cache(key as string, actualMethodName, oldVal, newVal); - } - } - }, - }); +function initialize( + instance: T, + propertyName: string & keyof T, + callback: ChangeCallback, +) { + const controller = new PropertyObserverController(instance as T, { propertyName, callback }); + instance.addController(controller); } diff --git a/core/pfe-core/decorators/observes.ts b/core/pfe-core/decorators/observes.ts index 26b854f52c..485ddd7809 100644 --- a/core/pfe-core/decorators/observes.ts +++ b/core/pfe-core/decorators/observes.ts @@ -1,30 +1,40 @@ import type { ReactiveElement } from 'lit'; -import type { ChangeCallback } from '@patternfly/pfe-core/controllers/property-observer-controller.js'; +import { + PropertyObserverController, + type ChangeCallback, + type PropertyObserverOptions, +} from '@patternfly/pfe-core/controllers/property-observer-controller.js'; /** * Observes changes on the given property and calls the decorated method - * with the old and new values when it changes. + * with the old and new values when it changes. In cases where the decorated method + * needs to access uninitialized class fields, You may need to wait for the element to connect + * before running your effects. In that case, you can optionally specify which + * lifecycle state to wait for. e.g.: + * - `waitFor: 'firstUpdate'` waits until the first update cycle has completed + * - `waitFor: 'updated'` waits until the next update cycle has completed + * - `waitFor: 'connected'` waits until the element connects * @param propertyName property to react to + * @param [options] options including lifecycle to wait on. */ -export function observes(propertyName: string & keyof T) { - return function(proto: T, methodName: string) { - const method = proto[methodName as keyof T] as ChangeCallback; - if (typeof method !== 'function') { +export function observes( + propertyName: string & keyof T, + options?: Partial, 'waitFor'>>, +) { + return function(proto: T, methodName: string): void { + const callback = proto[methodName as keyof T] as ChangeCallback; + if (typeof callback !== 'function') { throw new Error('@observes must decorate a class method'); } - const descriptor = Object.getOwnPropertyDescriptor(proto, propertyName); - Object.defineProperty(proto, propertyName, { - ...descriptor, - configurable: true, - set(this: T, newVal: T[keyof T]) { - const oldVal = this[propertyName as keyof T]; - // first, call any pre-existing setters, e.g. `@property` - descriptor?.set?.call(this, newVal); - method.call(this, oldVal, newVal); - }, + const klass = proto.constructor as typeof ReactiveElement; + klass.addInitializer(instance => { + instance.addController(new PropertyObserverController(instance as T, { + ...options, + propertyName, + callback, + })); }); }; } - diff --git a/core/pfe-core/test/decorators.spec.ts b/core/pfe-core/test/decorators.spec.ts index c459043bbc..711b3606b1 100644 --- a/core/pfe-core/test/decorators.spec.ts +++ b/core/pfe-core/test/decorators.spec.ts @@ -3,15 +3,17 @@ import { fixture, expect } from '@open-wc/testing'; import { customElement } from 'lit/decorators/custom-element.js'; import { property } from 'lit/decorators/property.js'; import { observes } from '../decorators/observes.js'; +import { observed } from '../decorators/observed.js'; + +import { spy } from 'sinon'; @customElement('x-observes-host') class XObservesHost extends LitElement { @property({ type: Number }) count = 0; - old?: number; current?: number; - - @observes('count') numberChanged(old: number, current: number) { + @observes('count') + numberChanged(old: number, current: number) { this.old = old; this.current = current; } @@ -23,16 +25,112 @@ describe('@observes', function() { element = await fixture(html``); }); it('initializes with old and new values', function() { - expect(element.old).to.equal(undefined); - expect(element.current).to.equal(0); + expect(element.old, 'old').to.equal(undefined); + expect(element.current, 'current').to.equal(0); + }); + describe('setting the observed prop', function() { + beforeEach(function() { + element.count = 1; + }); + beforeEach(() => element.updateComplete); + it('updates old and new values', function() { + expect(element.old, 'old').to.equal(0); + expect(element.current, 'current').to.equal(1); + }); + }); +}); + +@customElement('x-observed-bare-host') +class XObservedBareHost extends LitElement { + @observed + @property({ type: Number }) count = 0; + + old?: number; + current?: number; + _countChanged(old: number, current: number) { + this.old = old; + this.current = current; + } +} + +describe('@observed', function() { + let element: XObservedBareHost; + beforeEach(async function() { + element = await fixture(html``); + }); + it('initializes with old and new values', function() { + expect(element.old, 'old').to.equal(undefined); + expect(element.current, 'current').to.equal(0); + }); + describe('setting the observed prop', function() { + beforeEach(function() { + element.count = 1; + }); + beforeEach(() => element.updateComplete); + it('updates old and new values', function() { + expect(element.old, 'old').to.equal(0); + expect(element.current, 'current').to.equal(1); + }); + }); +}); + +@customElement('x-observed-configured-host') +class XObservedConfiguredHost extends LitElement { + @observed('_myCallback') + @property({ type: Number }) count = 0; + + old?: number; + current?: number; + _myCallback(old: number, current: number) { + this.old = old; + this.current = current; + } +} + +describe('@observed(\'_myCallback\')', function() { + let element: XObservedConfiguredHost; + beforeEach(async function() { + element = await fixture(html``); + }); + it('initializes with old and new values', function() { + expect(element.old, 'old').to.equal(undefined); + expect(element.current, 'current').to.equal(0); + }); + describe('setting the observed prop', function() { + beforeEach(function() { + element.count = 1; + }); + beforeEach(() => element.updateComplete); + it('updates old and new values', function() { + expect(element.old, 'old').to.equal(0); + expect(element.current, 'current').to.equal(1); + }); + }); +}); + +describe('@observed(() => {...})', function() { + let element: XObservedFunctionHost; + const dump = spy(); + @customElement('x-observed-function-host') + class XObservedFunctionHost extends LitElement { + @observed(dump) + @property({ type: Number }) count = 0; + } + + beforeEach(async function() { + element = await fixture(html``); + }); + + it('initializes with old and new values', function() { + expect(dump).to.have.been.calledWith(undefined, 0); }); describe('setting the observed prop', function() { beforeEach(function() { - element.count++; + element.count = 1; }); + beforeEach(() => element.updateComplete); it('updates old and new values', function() { - expect(element.old).to.equal(0); - expect(element.current).to.equal(1); + expect(dump).to.have.been.calledWith(0, 1); }); }); }); From 30c8e3df10112a2b58a74ad594097d55d1e21ecf Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Wed, 10 Jul 2024 09:00:36 +0300 Subject: [PATCH 7/8] fix: observes calls --- elements/pf-accordion/pf-accordion.ts | 7 ++++--- elements/pf-tabs/pf-tab.ts | 8 ++++---- elements/pf-tabs/pf-tabs.ts | 6 ++++-- elements/pf-tabs/test/pf-tabs.spec.ts | 23 +++++++++++------------ 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/elements/pf-accordion/pf-accordion.ts b/elements/pf-accordion/pf-accordion.ts index e03ab9689a..f5e2795cc6 100644 --- a/elements/pf-accordion/pf-accordion.ts +++ b/elements/pf-accordion/pf-accordion.ts @@ -233,12 +233,13 @@ export class PfAccordion extends LitElement { */ async #init() { this.#initialized ||= !!await this.updateComplete; - // Event listener to the accordion header after the accordion has been initialized to add the roving tabindex - this.addEventListener('focusin', this.#updateActiveHeader); + // Event listener to the accordion header after the accordion + // has been initialized to add the roving tabindex this.updateAccessibility(); } - #updateActiveHeader() { + @listen('focusin') + protected updateActiveHeader(): void { if (this.#activeHeader !== this.#headerIndex.activeItem) { this.#headerIndex.setActiveItem(this.#activeHeader); } diff --git a/elements/pf-tabs/pf-tab.ts b/elements/pf-tabs/pf-tab.ts index 3a5352fc24..4adcfb4047 100644 --- a/elements/pf-tabs/pf-tab.ts +++ b/elements/pf-tabs/pf-tab.ts @@ -143,15 +143,15 @@ export class PfTab extends LitElement { } @observes('active') - protected activeChanged(old: boolean) { - this.#internals.ariaSelected = String(!!this.active); - if (this.active && !old) { + protected activeChanged(old: boolean, active: boolean): void { + this.#internals.ariaSelected = String(!!active); + if (active && !old) { this.#activate(); } } @observes('disabled') - protected disabledChanged() { + protected disabledChanged(): void { this.#internals.ariaDisabled = this.disabled ? 'true' : this.ariaDisabled ?? 'false'; } } diff --git a/elements/pf-tabs/pf-tabs.ts b/elements/pf-tabs/pf-tabs.ts index c95a111d65..8f406838f4 100644 --- a/elements/pf-tabs/pf-tabs.ts +++ b/elements/pf-tabs/pf-tabs.ts @@ -20,6 +20,7 @@ import { type PfTabsContext, TabExpandEvent, context } from './context.js'; import '@patternfly/elements/pf-icon/pf-icon.js'; import styles from './pf-tabs.css'; +import { observes } from '@patternfly/pfe-core/decorators/observes.js'; /** * **Tabs** allow users to navigate between views within the same page or context. @@ -168,8 +169,9 @@ export class PfTabs extends LitElement { this.ctx = this.#ctx; } - protected override updated(changed: PropertyValues): void { - if (changed.has('activeTab') && this.activeTab?.disabled) { + @observes('activeTab') + protected activeTabChanged(old?: PfTab, activeTab?: PfTab): void { + if (activeTab?.disabled) { this.#logger.warn('Active tab is disabled. Setting to first focusable tab'); this.activeIndex = 0; } diff --git a/elements/pf-tabs/test/pf-tabs.spec.ts b/elements/pf-tabs/test/pf-tabs.spec.ts index 016ebe19f6..2fa33bd9f5 100644 --- a/elements/pf-tabs/test/pf-tabs.spec.ts +++ b/elements/pf-tabs/test/pf-tabs.spec.ts @@ -323,23 +323,22 @@ describe('', function() { beforeEach(async function() { element = await createFixture(html` - Users - Users - Containers - Containers - Database - Database - Disabled - Disabled - Aria Disabled - Aria Disabled + + + + + + + + + + `); }); it('should activate the next focusable tab', function() { - const [, second] = element.querySelectorAll('pf-tab'); - expect(second).to.have.attribute('active'); + expect(element.activeTab).to.have.id('tab2'); }); }); }); From 791065ae261a5c3b7c98cde18fc9eec4060047af Mon Sep 17 00:00:00 2001 From: Benny Powers Date: Mon, 15 Jul 2024 14:31:56 +0300 Subject: [PATCH 8/8] fix(core): improved types --- core/pfe-core/decorators/listen.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/core/pfe-core/decorators/listen.ts b/core/pfe-core/decorators/listen.ts index a59faf720a..8377f77f24 100644 --- a/core/pfe-core/decorators/listen.ts +++ b/core/pfe-core/decorators/listen.ts @@ -6,14 +6,17 @@ import type { LitElement } from 'lit'; * @param type event type e.g. `click` * @param options event listener options object e.g. `{ passive: true }` */ -export function listen(type: string, options?: EventListenerOptions) { +export function listen

( + type: keyof HTMLElementEventMap, + options?: EventListenerOptions, +) { return function( proto: LitElement, methodName: string, ): void { const origConnected = proto.connectedCallback; const origDisconnected = proto.disconnectedCallback; - const listener = (proto as any)[methodName] as EventListener; + const listener = (proto as P)[methodName as keyof P] as EventListener; proto.connectedCallback = function() { origConnected?.call(this); this.addEventListener(type, listener, options);