feat(validation): warn on invalid enum values instead of overriding - #6564
Closed
miwha-adobe wants to merge 43 commits into
Closed
feat(validation): warn on invalid enum values instead of overriding#6564miwha-adobe wants to merge 43 commits into
miwha-adobe wants to merge 43 commits into
Conversation
Lays the foundation for consistent dev-mode validation warnings across 2nd-gen components: shared helpers (validateEnum, warnIf, validateRequiredSlot, validateAllowedChildren) in @spectrum-web-components/core/utils replace the hand-rolled includes()+warn() checks each component previously wrote independently. Also fixes window.__swc.warn's dedup key (adds the message to localName:type:level) so two distinct warnings on the same component no longer silently suppress each other. Documents the full set of validation categories (enum, required, conditionally required, mutually exclusive/no-effect, required slots, allowed children, component-specific quirks via warnIf) in the style guide and the washing-machine migration workflow, and adds a consumer- facing Storybook guide explaining how the warnings work.
The rest of the repo's docs use "2nd-gen" as an established convention (120+ files), so pre-existing prose in touched files is left as-is; this only fixes the one line of net-new prose this PR introduced.
Turns the "no guaranteed production stripping" known limitation into actionable guidance: concrete recipes for webpack/Next.js, Vite, esbuild, and Rollup to confirm process.env.NODE_ENV is actually replaced and the resulting dead branch removed, plus the no-bundler ReferenceError case. Addresses the concern that components could ship dev-only validation code and message strings to production bundles by default.
…elpers First representative batch applying the shared dev-validation helpers from the foundation branch: - Badge: variant enum check -> validateEnum; outline/non-semantic- variant combo -> warnIf. - Tabs: keyboard-activation/density/direction enum checks -> validateEnum (kept in their property setters alongside the existing fallback-value logic); required accessible-label check -> warnIf. - IllustratedMessage: size/orientation enum checks -> validateEnum; heading slot allowed-children check -> validateAllowedChildren (previously a one-off inline tag allowlist). Also fixes the docs URLs in these three components (and this session's style-guide examples) from the stale opensource.adobe.com/spectrum-web- components/components/* pattern to the actual Gen2 docs site: spectrum-web-components.adobe.com/?path=/docs/components-*--docs. No new required-slot check was added for IllustratedMessage's heading slot: an existing test explicitly asserts no warning fires when it's empty, so that's left alone rather than overridden. All existing tests for these three components pass unchanged (97 tests across badge/tabs/illustrated-message/dev-validation), since the new helpers preserve enough of each original message's wording for the existing assertions to still match.
VARIANTS_SEMANTIC has a fixed initializer directly on BadgeBase (SHARED API section), unlike VARIANTS (API TO OVERRIDE, no initializer, genuinely meant to differ per concrete subclass per its JSDoc). Going through this.constructor for VARIANTS_SEMANTIC was redundant indirection; the already-imported BADGE_VARIANTS_SEMANTIC constant is identical. The VARIANTS lookup stays, since that one is by design abstract and subclass-supplied.
… static" This reverts commit 2dc721c.
…eter - ProgressCircle: light-DOM-children deprecation and missing- accessible-name checks -> warnIf. New: staticColor enum check via validateEnum (previously undeclared/unvalidated despite STATIC_COLORS existing for styling purposes). - LinearProgressMixin (shared by Meter): value-out-of-range and missing-accessible-name checks -> warnIf, keeping the mixin's own instance-level _hasWarned* flags as-is (they implement a distinct "warn once per bad-state entry, reset on recovery" policy layered on top of the shared warn() dedup, not something the helpers replace). New: labelPosition and staticColor enum checks via validateEnum (previously undeclared/unvalidated). - Meter: variant enum check -> validateEnum. All existing tests for progress-circle and meter pass unchanged (48 tests).
- Button: pending+disabled combo and icon-only-without-label checks -> warnIf (the latter now tagged type: 'accessibility', matching how every other accessible-name check in this effort is categorized). - ActionButton: new staticColor enum check via validateEnum (previously undeclared/unvalidated; no core base class owns this property, so the check lives directly on the swc concrete class). - Card: variant/density enum checks -> validateEnum; title-as-link- without-a-link-element check -> warnIf. - Tooltip: dangling for="" reference check -> warnIf. New: variant and placement enum checks via validateEnum (both had a VARIANTS/PLACEMENTS static already declared but never actually validated against). color-loupe was reviewed and needs no changes (boolean + free-text CSS color property only, no enums/required/slots). All existing tests for action-button, button, button-group, tooltip, and card pass unchanged (142 tests).
- Asset: variant enum check -> validateEnum. - AlertBanner: variant enum check (in the property setter) -> validateEnum. - StatusLight: 'accent' deprecation and disabled-attribute-removed checks -> warnIf; variant enum check -> validateEnum. Fixed the variant check's stale opensource.adobe.com URL. - Popover: dangling for="" reference check and required accessible- label-on-show check -> warnIf; placement enum check -> validateEnum. New: size enum check via validateEnum (VALID_SIZES existed but was never validated). Updated one test (status-light) that asserted the exact old warning message text via .toBe() rather than .toContain(); switched it to match the new standardized validateEnum message format instead of hand-tuning the helper's wording to preserve a stale test string. All existing tests for asset, alert-banner, status-light, and popover pass (90 tests).
- Divider: staticColor enum check -> validateEnum. - Avatar: missing-alt accessibility check -> warnIf. New: numeric size enum check via warnIf (VALID_SIZES existed and the setter already fell back to the default on an invalid value, but never warned about it). - ButtonGroup: orientation and align enum checks -> validateEnum. Icon was reviewed and needs no changes (free-text label, no required slot semantics beyond a graceful no-op when nothing is slotted). All existing tests for divider, avatar, icon, and button-group pass unchanged (78 tests).
ACCORDION_DENSITIES existed as the canonical valid-values source but was never actually validated anywhere. Added via validateEnum. ColorLoupe and ColorHandle were both already reviewed earlier this effort and confirmed to need no changes (booleans + free-text CSS color only). All existing tests for accordion, color-handle, and color-loupe pass unchanged (67 tests).
…DEBUG Each helper in dev-validation.ts now returns immediately when process.env.NODE_ENV === 'production', ahead of the existing window.__swc?.DEBUG check. This makes the internal engine logic (dedup lookup, message formatting, the window.__swc.warn call) provably dead code to a bundler that replaces process.env.NODE_ENV and minifies, since it's now gated on a build-time-foldable literal rather than solely a runtime read on a global that no bundler can prove false across module boundaries. Caught during testing: an earlier version of this gate matched spectrum-element.ts's exact `=== 'development'` check, which broke every dev-validation test, because this repo's own Vitest environment sets NODE_ENV to 'test', not 'development'. Using `=== 'production'` (opt out of exactly production, rather than opt in to exactly development) is both correct and safer, since it only disables validation in a build that explicitly declares itself production, leaving development/test/anything-else unaffected. This does not remove call sites or their argument evaluation (still a small, bounded per-call cost), and this package's own build still does not perform the NODE_ENV substitution before publishing. Both are documented as remaining gaps in the "Known limitations" section, which also now correctly describes what stripping does and does not achieve (the previous wording overstated it). A full dual dev/prod build (React's require()-branch pattern) is noted as the more complete, much larger follow-up.
…n/feat-dev-warning-validation-components
…te cost State the call-site overhead as a fact, not a fact plus a reassurance that it's fine.
…n/feat-dev-warning-validation-components
… into ruben/feat-dev-warning-validation-foundation
… into ruben/feat-dev-warning-validation-foundation
… into ruben/feat-dev-warning-validation-foundation
…//github.com/adobe/spectrum-web-components into ruben/feat-dev-warning-validation-foundation
…n/feat-dev-warning-validation-components
Exposes the same NODE_ENV/DEBUG predicate emitWarning uses so call sites can guard warnings whose condition or message is expensive to compute (e.g. a DOM traversal) and skip that work when validation is off. Cheap conditions should still call warnIf/validateEnum directly, which gate internally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n/feat-dev-warning-validation-components
…n/feat-dev-warning-validation-components
…//github.com/adobe/spectrum-web-components into ruben/feat-dev-warning-validation-components
Proof-of-concept for the dev-warning fallback policy discussion. Removes the coerce-to-default behavior for constrained enums so an invalid value is left in place and a dev-mode warning is emitted, rather than being silently swapped for the default: - Tabs: keyboardActivation, density, direction (setters -> plain @Property + validateEnum in willUpdate) - AlertBanner: variant (setter no longer blanks/removes the attribute) - Meter: variant (drops the willUpdate this.variant = 'informative' fallback) Tests updated to assert the warn-only behavior. Attached to the fallback-vs-warn-only policy ticket for team review; not intended to merge until the policy is decided. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
16 tasks
Contributor
📚 Branch Preview Links🔍 Gen1 Visual Regression Test ResultsWhen a visual regression test fails (or has previously failed while working on this branch), its results can be found in the following URLs:
Deployed to Azure Blob Storage: If the changes are expected, update the |
miwha-adobe
added
do-not-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This is a proof-of-concept, not intended to merge. It demonstrates removing the "coerce an invalid value to a default" behavior for constrained enums, so that an invalid value is left in place and a dev-mode warning is emitted instead of being silently swapped for the default. The purpose is to make the trade-offs concrete for a team discussion on what our fallback policy should be.
Changes shown here (for illustration only):
keyboardActivation,density,direction): coercing getter/setter pairs replaced with plain@property+validateEnuminwillUpdate(warn-only).variant): the setter no longer blanks or removes the attribute on invalid input.variant): drops thewillUpdatethis.variant = 'informative'fallback.The open question
When a consumer sets an invalid property value, should the component fall back to a default (and warn), or leave the value as-is (and warn)? Our components are currently inconsistent about this, and we want one deliberate, documented policy.
The tension comes down to production behavior, since dev warnings are stripped from production builds:
variant="banana"matches no CSS rule, so the component renders unstyled, with no warning). Fallback-to-default means it always looks like something, and developers still get told in dev.Proposed policy to discuss (not decided)
A consistent matrix, applied the same way everywhere, rather than per-component choices:
variant,size,density,direction)progress,level)Two things to note about how the current code sits against this proposal:
"Loading") when no accessible name is provided. Under the a11y exception that would become warn-only. This is the sharpest trade-off in the whole policy, because in production (no warning) it would ship an unnamed element to end users. It needs the most discussion and is not changed here.Relationship to the placement PR
The philosophy-neutral part of this effort (moving dev-warnings to the pre-render phase) is separated into its own PR and does not depend on this decision. That PR carries no behavior change. This branch carries only the contested coercion change.
Do not merge
Blocked on the fallback-policy decision. Attached to [Jira ticket number] for team discussion.