refactor(v4): rename $destroy()/destroyed() to $unmount()/unmounted() - #863
Merged
Conversation
v4 has no destroyed state: `#isMounted` is one boolean with two values, and
the doc comment on `$destroy()` existed only to explain that destroy does not
destroy. `$mount()`/`$unmount()` and `mounted()`/`unmounted()` are symmetric
pairs where `$mount()`/`$destroy()` was not. Vue 3 made the same rename.
Scope is `packages/v4` and `packages/eslint-plugin`. v3 keeps `$destroy()` and
`destroyed()`; `packages/js-toolkit` and `packages/docs` are untouched.
- `Base`: `$destroy()`, the `destroyed()` hook, `#destroyCallbacks` and the
guard message. Both doc comments rewritten to state the semantics plainly
instead of arguing against the old name.
- `registry`: `destroyPair`/`destroyWithin`/`destroyRemovedSubtrees`, and the
withdrawn-declaration comment now names its two steps — leave the mount
cycle, then drop the identity.
- `events`: `js-toolkit:component:destroyed` is now
`js-toolkit:component:unmounted`, the only wire-format change. v4 is
unreleased.
- `MountStrategyHooks` is `{ mount, unmount }`: the hook drives `$unmount()`.
- The service mixin, six migration components, every spec, bench and demo.
Kept: `smoothTo().destroy()` and the storage handle's `destroy()`, which tear
down standalone objects with no mount cycle to leave; and the `DESTROYED_EVENT`
absence assertion in `exports.spec.ts`, about a name that never shipped.
Adds `no-destroy-lifecycle` to the eslint plugin's `v4` config, autofixing both
names. It is its own rule rather than an entry in `no-deprecated-properties`,
which only flags a member expression whose object is `this` — and the common
call is `instance.$destroy()`. `V4_REMOVED` and `V4_REMOVED_METHODS` now point
`$terminate` and `terminated` at the names v4 has.
Recorded as gap 48 in migration/REPORT.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM
Export sizeBundled per export with peer dependencies left external, dynamic imports excluded and the output minified; sizes are gzipped. @studiometa/js-toolkit-v4
Unchanged (375)@studiometa/js-toolkit
@studiometa/js-toolkit-v4
|
Code ReviewRisk: Low — The rename is internally consistent across the v4 lifecycle, registry, events, services, migration components, tests, and ESLint tooling; safe to merge. This change renames the reversible v4 lifecycle API from I reviewed the complete commit diff, including the v4 Review usage: 241,233 in (182,940 cached) / 801 out tokens — $0.0480 (openrouter/openai/gpt-5.6-luna, thinking: low) Reviewed by @weareikko/code-review v0.9.5 for commit 501a5d0. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #863 +/- ##
==========================================
+ Coverage 97.04% 97.06% +0.01%
==========================================
Files 175 176 +1
Lines 4535 4561 +26
Branches 1322 1330 +8
==========================================
+ Hits 4401 4427 +26
Misses 122 122
Partials 12 12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
v4 mount benchmarksBase and head measured on this runner, alternating over 3 rounds each; every value is the median of the round medians. Running both sides on one machine is what removes cross-machine noise — a cached baseline from another runner would put it back. A move under 25%, or on a benchmark under 5 ms, is not reported as a change: it is inside the measured noise of a shared runner.
Within noise (18)
|
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
1 participant
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.
Why
Base.$destroy()was documented as "the reversible inverse of$mount(). The instance stays on its element and can mount again." A method whose doc comment has to explain that it does not destroy anything is misnamed. There is no destroyed state —#isMountedis one boolean with two values.$mount()/$unmount()andmounted()/unmounted()are symmetric pairs.$mount()/$destroy()is not. Vue 3 made this same rename (beforeDestroy/destroyed→beforeUnmount/unmounted) for the same reason; React hascomponentWillUnmount, custom elements havedisconnectedCallback.v4 is unreleased, so this is the only time the rename is free.
What changed
Base.$destroy()→$unmount(), thedestroyed()hook →unmounted(),#destroyCallbacks→#unmountCallbacks.registry.ts:destroyPair/destroyWithin/destroyRemovedSubtrees→unmount*.MountStrategyHooks{ mount, destroy }→{ mount, unmount }. Internal, not re-exported — but leaving it would reintroduce the same asymmetry one layer down.js-toolkit:component:destroyed→js-toolkit:component:unmounted(EVENTS.component.unmounted).New ESLint rule:
js-toolkit/no-destroy-lifecycleAutofixes
$destroy()→$unmount()anddestroyed()→unmounted(). Registered inconfigs.v4only — v3 keepsdestroyed().It is a dedicated rule rather than an entry in
no-deprecated-properties, for three reasons:thisreceivers so unrelatedsomeLibrary.$parentis not noise. The common call isinstance.$destroy(), so folding in would miss the common case, and lifting the guard for one entry lifts it for all eleven.$parent→$closest()and$children→$watchChildren()are not safe textual rewrites. A rename is the one case where a fixer is exactly right.The two halves are deliberately asymmetric.
$destroyis flagged on any receiver (the$prefix is the toolkit's namespace and the rule is opt-in viaconfigs.v4);destroyedhas no prefix to lean on, so it is narrowed to a non-staticdestroyed()method definition in a class that reads as a v4 component, plussuper.destroyed()inside one.foo.destroyed()is untouched, and computed access is skipped.Also updated:
V4_REMOVED$terminate→$unmount(),V4_REMOVED_METHODSterminated→unmounted().Deliberately unchanged
smoothTo().destroy()and the storage handles. Standalone objects with no mount cycle and no second life — the final word is the accurate one there. Renaming them would say the opposite of what this PR says.require-destroyed-cleanupandasync-lifecycle-methods— both v3-only.migration/REPORT.mdprose, which records what the names were at the time. Appended as gap 48 instead.AnchorNav.ts,Menu.tsandRATIONALE.md, reverted after a mechanical sweep had rewritten statements about v3.children-watchersshared-runtime-slot revision. A field inside it was renamed, but the revision guards two released copies coexisting and v4 has never released a revision 1.Verification
packages/v4npm run testpackages/v4npm run lint:typespackages/v4npm run check:diagnosticspackages/v4npm run build+check:constant-subpathspackages/eslint-pluginnpm testpackages/eslint-pluginnpm run lint:typeslint:fmt,subpaths:checknpm run lint:staticreports two pre-existingunicorn(no-useless-spread)warnings injs-toolkit/src/autoload/loader.ts:467andv4/src/context.ts:49. Neither file is touched here.Known unrelated failure:
packages/v4/scripts/check-package.jsthrowsTypeError: object is not iterableat line 288 — npm 12 returnsnpm pack --jsonas an object keyed by package name, while the script destructures an array. Fixed separately in #862.🤖 Generated with Claude Code
https://claude.ai/code/session_01NnepYhqjPMcFCSb43PoHRM