-
Notifications
You must be signed in to change notification settings - Fork 257
feat(validation): lifecycle placement of dev warnings #6561
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5b6600a
2603a2a
3dbd430
3c78374
369a189
9069214
e34c64b
8b98b90
c4cdbce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -471,10 +471,7 @@ export abstract class TooltipBase | |
| if (this.disabled && this.open) { | ||
| this.open = false; | ||
| } | ||
| } | ||
|
|
||
| protected override updated(changedProperties: PropertyValues): void { | ||
| super.updated(changedProperties); | ||
| if (changedProperties.has('variant')) { | ||
| const constructor = this.constructor as typeof TooltipBase; | ||
| validateEnum(this, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a bug: I see Tooltip and ProgressCircle fold the validateEnum check in |
||
|
|
@@ -493,6 +490,10 @@ export abstract class TooltipBase | |
| url: 'https://spectrum-web-components.adobe.com/?path=/docs/components-tooltip--docs', | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| protected override updated(changedProperties: PropertyValues): void { | ||
| super.updated(changedProperties); | ||
| if (changedProperties.has('offset')) { | ||
| this.style.setProperty( | ||
| '--_swc-tooltip-animation-distance', | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| import { Canvas, Meta } from '@storybook/addon-docs/blocks'; | ||
| import { DocsFooter, DocsHeader } from '../../../swc/.storybook/blocks'; | ||
|
|
||
| import * as Stories from './stories/attribute-observer-controller.stories'; | ||
|
|
||
| <Meta of={Stories} /> | ||
|
|
||
| <DocsHeader /> | ||
|
|
||
| ## Usage | ||
|
|
||
| `AttributeObserverController` watches a set of host attributes and requests a host re-render whenever one of them changes. It exists for state that lives in plain HTML attributes rather than reactive properties. `aria-label` and `aria-labelledby` are the motivating case: they are not declared as Lit `@property`s, so mutating them after connect does not trigger Lit's update cycle, and any `updated()` logic that reads them would otherwise run only once and then go stale. | ||
|
|
||
| Like [Slot presence controller](../?path=/docs/core-controllers-slot-presence-controller--docs), it uses a `MutationObserver` and calls `host.requestUpdate()` on change; the host's own `updated()` then re-runs and re-reads the attributes. The controller holds no state and makes no decisions about the attributes it watches. | ||
|
|
||
| ```typescript | ||
| import { LitElement, html, type PropertyValues } from 'lit'; | ||
| import { AttributeObserverController } from '@adobe/spectrum-wc-core/controllers'; | ||
| import { isDebug } from '@adobe/spectrum-wc-core/utils'; | ||
|
|
||
| class SwcThing extends LitElement { | ||
| constructor() { | ||
| super(); | ||
| new AttributeObserverController(this, ['aria-label', 'aria-labelledby'], { | ||
| debugOnly: true, | ||
| }); | ||
| } | ||
|
|
||
| protected override updated(changes: PropertyValues): void { | ||
| super.updated(changes); | ||
| if (isDebug()) { | ||
| this.warnIfMissingAccessibleName(); | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Use `debugOnly` when the only consumer is a dev warning | ||
|
|
||
| Pass `{ debugOnly: true }` when the sole reason to watch the attributes is to re-run a development-mode check. The observer is then attached only while `isDebug()` is `true`, so it costs nothing in production (and nothing during SSR). When the attribute changes drive real runtime behavior, leave `debugOnly` unset so the observer always runs. | ||
|
|
||
| ## Behaviors | ||
|
|
||
| ### Revalidate on attribute change | ||
|
|
||
| Both boxes below read their own `aria-label` and render it. Only the left one runs the controller. The button changes `aria-label` on both from the outside. The box with the controller notices the change, re-renders, updates its value, and shows a ✓ confirmation that its follow-up ran. The box without it stays frozen on the old value and never confirms, because nothing tells Lit the attribute changed. That difference is the controller's entire job: turn an out-of-band attribute change into a normal re-render so `updated()` can react. | ||
|
|
||
| <Canvas of={Stories.RevalidateOnChange} /> | ||
|
|
||
| ## Accessibility | ||
|
|
||
| The controller has no direct accessibility surface. Its common use is to keep an accessible-name check current when the name is supplied via `aria-label` / `aria-labelledby`, but the controller only triggers re-evaluation; the host owns the actual check and message. | ||
|
|
||
| ## API | ||
|
|
||
| ### Constructor | ||
|
|
||
| `new AttributeObserverController(host, attributes, options?)` registers the controller on the host. | ||
|
|
||
| ### Parameters | ||
|
|
||
| | Parameter | Type | Description | | ||
| | ------------ | ------------------------------------ | ---------------------------------------------------------------------------------------------- | | ||
| | `host` | `ReactiveElement` | The element that owns the controller. | | ||
| | `attributes` | `string \| string[]` | The attribute name(s) to observe (the observer's `attributeFilter`). | | ||
| | `options` | `AttributeObserverControllerOptions` | Optional. `debugOnly` (default `false`): only attach the observer while `isDebug()` is `true`. | | ||
|
|
||
| <DocsFooter /> |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /** | ||
| * Copyright 2026 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| export { | ||
| AttributeObserverController, | ||
| type AttributeObserverControllerOptions, | ||
| } from './src/attribute-observer-controller.js'; |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this controller? It feels like a big overhead for what could (maybe) be simpler... e.g., the platform already gives us But even setting that aside (and following up our Slack thread), we already have a first-party pattern for this, using Aaaand as a bit of a tangent, but what I think we haven't decided / established a pattern is for |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| /** | ||
| * Copyright 2026 Adobe. All rights reserved. | ||
| * This file is licensed to you under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. You may obtain a copy | ||
| * of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software distributed under | ||
| * the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS | ||
| * OF ANY KIND, either express or implied. See the License for the specific language | ||
| * governing permissions and limitations under the License. | ||
| */ | ||
|
|
||
| import type { ReactiveController, ReactiveElement } from 'lit'; | ||
|
|
||
| import { isDebug } from '@adobe/spectrum-wc-core/utils/index.js'; | ||
|
|
||
| /** | ||
| * Options for {@link AttributeObserverController}. | ||
| */ | ||
| export interface AttributeObserverControllerOptions { | ||
| /** | ||
| * When `true`, the observer is only attached while dev-mode validation is | ||
| * active (`isDebug()`), so it costs nothing in production. Use this when the | ||
| * only reason to watch the attributes is to re-run a dev warning. Default: | ||
| * `false` (always observe). | ||
| */ | ||
| debugOnly?: boolean; | ||
| } | ||
|
|
||
| /** | ||
| * A reactive controller that watches a set of host attributes and requests a | ||
| * host re-render whenever one of them changes. It exists for state that lives | ||
| * in plain HTML attributes rather than reactive properties, for example | ||
| * `aria-label` / `aria-labelledby`: because those are not declared as Lit | ||
| * `@property`s, mutating them does not trigger Lit's update cycle, so any | ||
| * `updated()` logic that reads them would otherwise go stale. | ||
| * | ||
| * Like {@link SlotPresenceController}, it observes with a `MutationObserver` and | ||
| * calls `host.requestUpdate()` on change; the host's own `updated()` then | ||
| * re-runs and re-reads the attributes. The controller intentionally holds no | ||
| * state and makes no decisions about the attributes it watches. | ||
| * | ||
| * @example | ||
| * ```typescript | ||
| * // Re-run a dev-only accessible-name warning when the aria attributes change. | ||
| * class MyComponent extends SpectrumElement { | ||
| * private nameObserver = new AttributeObserverController( | ||
| * this, | ||
| * ['aria-label', 'aria-labelledby'], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because of cross root ARIA issues, hosts shouldn't be using the |
||
| * { debugOnly: true } | ||
| * ); | ||
| * | ||
| * protected override updated(changes: PropertyValues): void { | ||
| * super.updated(changes); | ||
| * if (isDebug()) { | ||
| * this.warnIfMissingAccessibleName(); | ||
| * } | ||
| * } | ||
| * } | ||
| * ``` | ||
| */ | ||
| export class AttributeObserverController implements ReactiveController { | ||
| private host: ReactiveElement; | ||
| private attributeFilter: string[]; | ||
| private debugOnly: boolean; | ||
| private observer: MutationObserver; | ||
|
|
||
| constructor( | ||
| host: ReactiveElement, | ||
| attributes: string | string[], | ||
| options: AttributeObserverControllerOptions = {} | ||
| ) { | ||
| this.host = host; | ||
| this.attributeFilter = Array.isArray(attributes) | ||
| ? attributes | ||
| : [attributes]; | ||
| this.debugOnly = options.debugOnly ?? false; | ||
| this.host.addController(this); | ||
|
|
||
| this.observer = new MutationObserver(() => { | ||
| // Lit batches `requestUpdate()` into the next microtask, so several | ||
| // attribute changes in one tick still coalesce into a single re-render. | ||
| this.host.requestUpdate(); | ||
| }); | ||
| } | ||
|
|
||
| hostConnected(): void { | ||
| // When only used to drive a dev warning, skip observing entirely in | ||
| // production (and SSR) so there is zero runtime cost there. | ||
| if (this.debugOnly && !isDebug()) { | ||
| return; | ||
| } | ||
| this.observer.observe(this.host, { | ||
| attributes: true, | ||
| attributeFilter: this.attributeFilter, | ||
| }); | ||
| } | ||
|
|
||
| hostDisconnected(): void { | ||
| this.observer.disconnect(); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is duplicating what is below (lines 241-248)
https://github.com/adobe/spectrum-web-components/pull/6561/changes#diff-2194b94b4d869a61170a29cb27f1ce8c70cad533d6e1f21ea90d8be4687cb2b9R241-R248