feat(dev-validation): add reusable dev-mode validation helpers and docs - #6508
feat(dev-validation): add reusable dev-mode validation helpers and docs#6508rubencarvalho wants to merge 22 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.
🦋 Changeset detectedLatest commit: 397d9ec The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📚 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 |
Coverage Report for CI Build 30485771433Coverage remained the same at 96.241%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
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.
…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.
…te cost State the call-site overhead as a fact, not a fact plus a reassurance that it's fine.
… into ruben/feat-dev-warning-validation-foundation
| * The version of the core base package. | ||
| */ | ||
| export const coreVersion = '0.3.0'; | ||
| export const coreVersion = '2.0.0-beta.2'; |
There was a problem hiding this comment.
I am not sure where this came from. Is this the right call to update this version to 2.0.0-beta.2? Our latest release was Since 2.0.0-beta.1
There was a problem hiding this comment.
we actually should make a ticket to remove this and disable the script thats updating it. its no longer coupled.
| * is deduplicated. Exported so the dedup key can be unit tested without relying | ||
| * on the `NODE_ENV`-gated `window.__swc` setup below. | ||
| */ | ||
| export function warningId( |
There was a problem hiding this comment.
Extracted warningId so the dedup-key fix could be unit tested
|
|
||
| ## Configuring which warnings you see | ||
|
|
||
| Each distinct warning message fires once per element type per page load, not once per problem occurrence; this keeps a single repeated mistake from flooding your console. Two different problems on the same component (even ones that share a warning type and severity) each get their own warning; the one case that's still collapsed is **multiple instances of the same component with the identical problem**: ten badges all set to the same invalid `variant` produce one console warning, not ten, from whichever badge updates first. |
There was a problem hiding this comment.
Trade off: any mid-iteration or a re-introduced mistake goes silent after the first time. Is it worth the trade off?
There was a problem hiding this comment.
i would argue no its not worth the trade off. a warning should fire always and if its making the console noisy, thats the point, fix this code. it also can help us debug rendering issues if something is firing too much or not enough. all mistakes should be documented and communicated.
the one case that's still collapsed is multiple instances of the same component with the identical problem: ten badges all set to the same invalid
variantproduce one console warning, not ten, from whichever badge updates first.
The message in our warnings includes where this error occurs. i would be irrated if i was only warned once, fix it in the spot the message told me and then get the same message again and have to do that 10 more times to clear all warnings. I would rather have a list of all issues, fix it and then rerun my server to check all warnings are clear.
we shouldnt be making the dev experience easier to work with warnings, dev mode is when everything should be firing and getting fixed before prod. by hiding any message we are doing a disservice in my opinion.
There was a problem hiding this comment.
Following up on my earlier comment: I misread the doc, so please disregard my "goes silent after the first time" worry. I verified the actual behavior which is:
-
Errors surface on every update as you develop, not just once on initial render.
-
A page reload also resets the dedup, so nothing goes quiet while you iterate. This was my main concern about things being missed or going silent.
-
Four components with the same invalid value produce 1 warning, not 4 (deduped by message). I am alright with this pattern personally, though I can see the argument in wanting to intentionally make it noisy so the developer fixes the issues. As a thought, perhaps there is something we could do to say "You have one or more instances of error xyz.....".
My biggest concern was mid-iteration or reintroduced issues being un-captured. But I did some manual testing and that doesn't seem to be the case.
There was a problem hiding this comment.
@caseyisonit @miwha-adobe I feel you both are right about a different problem.
Only warn once means you have 10 broken badges, we point you at one, you fix it, reload and we point you at the next one. Ten times. @caseyisonit your complaint is right and its fair here.
Now warning every time means a table with 500 rows bound to one bad valye gives you 500 console messages each holding onto a live DOM element. The devtools will lock up here for just one bug.
I would recommend we warn once per broken element but group them into a single console line. One message with all ten elements listed inside it and a count. This way we fix one root cause and we already knew there were ten.
There was a problem hiding this comment.
This is what I am proposing (example just for viewing. Not commiting the storybook instance)
| return `${localName}:${type}:${level}:${message}` as BrandedSWCWarningID; | ||
| } | ||
|
|
||
| if (process.env.NODE_ENV === 'development') { |
There was a problem hiding this comment.
While "development" seems like standard practice, if a consumer stores env as 'dev', 'prod', 'staging', or NODE_ENV unset, this will fail and falls into a gap: no warnings appear, and the code isn't stripped either (it runs and no-ops).
There was a problem hiding this comment.
i would be down to extend this to cover potential gaps, great call out about staging too. prod and production should turn everything off.
There was a problem hiding this comment.
@miwha-adobe @caseyisonit Only the exact word production can actually be deleted from our bundle. That's the specific string every build tool will look for.
prod can be made to behave correctly with no warnings but the code will still ship. This is something we cannot fix from inside the package, we need to amplify this saying in the consumer docs so nobody assumed they are equivalent.
There was a problem hiding this comment.
@Rajdeepc curious for your take.
We enable warnings on NODE_ENV === 'development' but strip on === 'production'. Anything in between (staging, test, unset, non-standard dev) gets no warnings and no stripping.
Do consumers realistically use values like dev, and do we want warnings in staging?
There was a problem hiding this comment.
Does anyone really use dev? test should be hit automatically by vitest and Jest, I am not sure about unset thought.
Also do we want warnings in staging, we cant answer this from inside the package. Its a deploy target not a build type. What we can fix is that there is currently no switch. window.__swc.warn only exists inside the development branch, so DEBUG = true does nothing anywhere else. This is a bug regardless of which strings we enumerate.
I would recommend we flip the default to be on unless production.
There was a problem hiding this comment.
I agree with that assessment, and good catch about the bug! That is something I did not consider.
… 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
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>
miwha-adobe
added
2nd gen
5 participants
Description
Adds the shared foundation for dev-mode validation warnings in 2nd-gen components: reusable helpers, one gating/dispatch path, a dedup fix, and docs. Foundation only — no component behavior changes. Applying the helpers to components is a follow-up branch.
New helpers in
@spectrum-web-components/core/utils, replacing the hand-rolledincludes()+window.__swc.warn()checks each component wrote on its own:validateEnum,validateRequiredSlot,validateAllowedChildrenwarnIf— a general predicate that also covers required and conditionally-required properties and mutually-exclusive combinationsAll warn in dev and no-op in production (via the
process.env.NODE_ENVgate). They surface an invalid value with a console warning; they do not change it.One dispatch path: every helper routes through a single private
emitWarning, so there is one source of truth for how a warning is gated and emitted.Dedup fix: the
window.__swc.warnkey now includes the message (localName:type:level→…:message), so two different warnings on the same component no longer suppress each other. Known limitation: identical messages across multiple instances of one component still collapse to one (escape hatch:window.__swc.verbose = true).isDebug()helper: gates expensive call-site checks (e.g. DOM traversals) so they do not run in production.Docs and process: a Debug and validation style-guide page (all validation categories, a lifecycle-hook quick-reference, and a "Known limitations" section); a consumer-facing "Dev mode warnings" Storybook guide; and validation checklist items added to the migration workflow and the
code-conformancerule.Out of scope (tracked as follow-ups)
warn()calls).process.env.NODE_ENVgate unreplaced.Motivation and context
Dev-warning validation exists but is inconsistently applied (~4 components, each with hand-rolled checks), and several requested categories (conditionally-required properties, mutually-exclusive combinations, required slots, allowed children) had no shared helper at all. This PR builds the reusable, documented foundation so the next phase — applying validation to every component — is mechanical and consistent instead of another one-off per component.
Related issue(s)
Screenshots (if appropriate)
N/A — no visual changes. New Storybook guide page: "Guides/Dev mode warnings".
Author's checklist
Reviewer's checklist
patch,minor, ormajorfeaturesManual review test cases
New helpers behave correctly
2nd-gen/packages/core/utils/test/dev-validation.test.tsin Storybook (or runyarn vitest --run --project storybook ../core/utils/test/dev-validation.test.tsfrom2nd-gen/packages/swc)Dedup fix doesn't break existing warning tests
yarn vitest --run --project storybook components/badge components/tabs components/progress-circle components/illustrated-messagefrom2nd-gen/packages/swcNew Storybook guide renders
yarn storybookfrom2nd-gen/packages/swc)Device review
Accessibility testing checklist
VO+Cmd+H, NVDAH).