Repository navigation
fix(scroller): don't throw from scroll operations before an element is set - #324
Merged
Merged
Conversation
…s set
Scroll operations (`by()`, `to()`, `toElement()`, `toSide()`, `toTop()`
etc.) built their options through `_checkElement()`, which threw `missing
element` when no element had been set. `useScrollerControl({ el: 'self' })`
only sets the element in `onMounted`, so calling them while a `<Suspense>`
boundary was still pending threw. They now return a result that does
nothing and resolves immediately.
`pushScrollStopper()` / `removeScrollStopper()` threw the same way. A
stopper pushed before the element is set is now kept and applied once the
element is set.
Release @fastkit/vue-scroller as well so its published range requires the
fixed @fastkit/scroller.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6
🦋 Changeset detectedLatest commit: 946da15 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 |
schwarz9791
commented
Oct 16, 2026
schwarz9791
left a comment
Collaborator
Author
There was a problem hiding this comment.
コードレビューで見つかった指摘を検証し、残ったものをインラインでまとめました。ブロッカーはありません。
…ing it `_checkElement()` could no longer throw: every caller ran only after `_runScroll()`, `_updateScrollable()` or `destroy()` had made sure an element exists. Pass that element to the option builders as an argument and inline `_scrollEnable()` / `_scrollDisable()`, so code that needs the element has to get it from one of those guards. Also cover scroll operations after `destroy()` in the tests. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6
schwarz9791
commented
Oct 16, 2026
schwarz9791
left a comment
Collaborator
Author
There was a problem hiding this comment.
76a6dc9 を再レビューしました。指摘への対応はどれも妥当で、追加の指摘はありません。scroller の typecheck / lint / test(16 件)が通ることも手元で確認しました。
schwarz9791
marked this pull request as ready for review
October 16, 2026 05:02
nkenji09
reviewed
Oct 16, 2026
nkenji09
reviewed
Oct 16, 2026
nkenji09
reviewed
Oct 16, 2026
nkenji09
reviewed
Oct 16, 2026
nkenji09
reviewed
Oct 16, 2026
nkenji09
approved these changes
Oct 16, 2026
nkenji09
left a comment
Collaborator
There was a problem hiding this comment.
この PR が入れた不具合は見つかりませんでした。PR の前からある不具合を 5 件、参考としてインラインに添えています(別対応で構いません)。
`pushScrollStopper()` on a destroyed instance no longer throws since the element check was relaxed, but it still recorded the stopper and turned `scrollEnabled` false. Ignore it instead, like the other calls after `destroy()`. `removeScrollStopper()` already does nothing then, as `destroy()` empties the stoppers. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6
dadajam4
approved these changes
Oct 19, 2026
Merged
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.
Summary
Scrollerthrewmissing elementwhen it was operated before an element had been set.@fastkit/vue-scrollerreaches this:useScrollerControl({ el: 'self' })(andVScroller) only sets the element inonMounted, so while a<Suspense>boundary is still pending, the control exists but itsScrollerhas no element yet.Scroll operations
by(),to(),toElement(),toSide(),toTop()and the other side/corner methods built their options through_checkElement(), which threw. All of them go through one private method, which now returns a result that does nothing and resolves immediately when there is no element (noonCancel/onDone). The options are only built after that check.Scroll stoppers
pushScrollStopper()/removeScrollStopper()threw the same way via_updateScrollable(). It now skips when there is no element; the stopper stays recorded and_setup()applies it once the element is set. On a destroyed instancepushScrollStopper()ignores the stopper, soscrollEnabledstays as it is.Since
destroy()drops the element, these calls afterdestroy()also do nothing instead of throwing. This matches the existing setters (scrollTopetc.), which already return when there is no element.Tests
packages/scroller/src/__tests__/scroll-without-element.client.spec.tsScrollerwithout an element: does not throw,promiseresolves,plansis{ x: 0, y: 0 }destroy():promiseresolves,plansis{ x: 0, y: 0 }pushScrollStopper()afterdestroy():scrollEnabledstaystrueAll of them fail before the fix and pass after it.
Release
@fastkit/scrollerpatch@fastkit/vue-scrollerpatch, so its published range becomes^1.0.2and updating it brings the fix along (see "Delivering a fix in a dependency to its dependents" indocs/release.md)🤖 Generated with Claude Code
https://claude.ai/code/session_015hoZxV4JFvpJpTzs5ZxqH6