Repository navigation
build(deps): drop scroll-to-element and lodash.throttle runtime deps - #66
Merged
Merged
Conversation
Replace scroll-to-element (unmaintained since 2018) with the native element.scrollIntoView(). The legacy align option (top/middle/bottom) is mapped to block (start/center/end); native behavior/block/inline options are forwarded as-is. Legacy offset/duration/ease options are accepted but ignored (no native equivalent) — semi-breaking, targets 0.7.0. Replace the deprecated lodash.throttle per-method package with a minimal local throttle (leading + trailing edges, cancel()) in src/lib/Form/throttle.js, with dedicated fake-timer tests. jsdom does not implement scrollIntoView: tests define a mock on Element.prototype and assert the mapped options. Refs #57
hugomallet
commented
Aug 6, 2026
hugomallet
left a comment
Collaborator
Author
There was a problem hiding this comment.
Self-review (assisted by Claude, review53 skill) — posted as COMMENT since I authored the PR.
Verdict: Approve with minor comments ✅
Scope strictly respected (7 files: src/lib/Form/* + package.json + yarn.lock). ESLint + tsc --noEmit clean on changed files. yarn.lock removes exactly the 3 announced packages plus their now-orphaned transitives (@types/lodash, the raf@^3.4.0 specifier from scroll-to-element).
Cleared after verification:
throttle.jsreproduces the lodash contract the form relies on: leading edge, trailing coalesced with the latest args, no spurious trailing after a single call,cancel()resets to a fresh leading edge (same as lodash, which zeroeslastInvokeTimeon cancel).flush()is not used anywhere, so dropping it is safe.- No timer leak:
componentWillUnmountandmemoGetValidatorboth callcancel(), which clears the pending timeout. scrollOptionsback-compat:scrollOptions || {}guards null; legacy{offset, align, duration, ease}doesn't crash —alignmaps toblock(explicitblockwins), the rest is ignored as documented in theJfvScrollOptionsJSDoc.defaultPropsmatches the updated snapshot.- Tests:
scrollIntoViewmocked onElement.prototype(per-file jsdom,mockClearinbeforeEach— no cross-test pollution); fake timers restored inafterEach.
🟡 Minor (grouped)
Form.js:377— no guard onelement.scrollIntoView. jsdom doesn't implement it (this PR's own tests mock it for exactly that reason), so a consumer's jsdom test that submits an invalid form now throwsTypeError: element.scrollIntoView is not a function, wherescroll-to-elementpreviously degraded to a harmless "Not implemented" console warning. The throw also skips theelement.focus({ preventScroll: true })a11y focus from #61. Suggested:if (typeof element.scrollIntoView === 'function') element.scrollIntoView({ behavior, block, inline });and keepfocus()unconditional.- Old Safari note (< 15.4):
scrollIntoViewOptionsis unsupported there — an object argument behaves likescrollIntoView(truthy)(instant, align top). Degradation only, no crash; worth one line in the 0.7.0 release notes. Form.test.js: consider anafterAllthat deletes theElement.prototype.scrollIntoViewmock — harmless today with Jest's per-file environment, just hygiene.
No blocking issue; item 1 is cheap and worth doing before merge.
… tests Review follow-up (#66): - Skip scrolling when the element does not implement scrollIntoView (e.g. consumers' jsdom test environments) instead of throwing; the a11y focus move still runs. Covered by a dedicated test. - Restore/remove the Element.prototype.scrollIntoView mock in afterAll.
24 tasks done
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
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.
Part of #57 (§3) — removes two dead runtime dependencies.
Changes
scroll-to-element→ nativeelement.scrollIntoView()— the package has been unmaintained since ~2018.scrollToFirstErrorkeeps its guards and thefocus({ preventScroll: true })call from fix(Form): check every name in isFieldInvalid; guard and focus scrollToFirstError #61. Legacyalign(top/middle/bottom) is mapped toblock(start/center/end); native{ behavior, block, inline }options are also accepted and forwarded as-is. Defaults:behavior: 'smooth',block: 'center',inline: 'nearest'. The call is guarded bytypeof element.scrollIntoView === 'function': environments that do not implement it (e.g. consumers' jsdom test setups) skip scrolling without throwing, and the a11y focus move still runs.lodash.throttle→ localsrc/lib/Form/throttle.js— the per-method lodash packages are deprecated. Minimal implementation reproducing the semantics the form relies on: leading + trailing edges, coalesced calls,cancel()(used bycomponentWillUnmountandmemoGetValidator). Dedicated fake-timer tests. TheDebouncedFuncJSDoc type from lodash is replaced by the localThrottledFunc.scroll-to-element,lodash.throttle(dependencies) and@types/lodash.throttle(devDependencies);yarn.lockupdated.Semi-breaking (targets 0.7.0)
scrollOptionsoffset,durationandeaseare still accepted but ignored: the native API has no equivalent — scroll animation is delegated to the browser viabehavior: 'smooth'. Custom animation duration/easing is lost. Documented in theJfvScrollOptionsJSDoc. 0.7.0 is already required by the peerDeps narrowing of feat(FieldError): announce errors via role="alert" + fix react peerDep #59.scrollIntoViewignores the options object there (treated asscrollIntoView(truthy)), resulting in an instant scroll aligned to top instead of a smooth centered scroll. Graceful degradation, no crash.Tests
scrollIntoView: tests mock it onElement.prototype(restored inafterAll) and assert the exact options object passed (defaults, legacyalignmapping with ignored extras, native passthrough), plus a test proving that an element withoutscrollIntoViewdoes not throw and still receives focus.CI=true yarn test:runtime);yarn type-checkclean. Snapshot updated for the newscrollOptionsdefault only.