Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(useScroll): add onError hook and avoid throws by default, fix #3580 #3605

Merged
merged 12 commits into from
Feb 20, 2024
34 changes: 24 additions & 10 deletions packages/core/useScroll/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { computed, reactive, ref } from 'vue-demi'
import type { MaybeRefOrGetter } from '@vueuse/shared'
import { noop, toValue, tryOnMounted, useDebounceFn, useThrottleFn } from '@vueuse/shared'
import { useEventListener } from '../useEventListener'
import type { MaybeElementRef } from '../unrefElement'
import { unrefElement } from '../unrefElement'
import type { ConfigurableWindow } from '../_configurable'
import { defaultWindow } from '../_configurable'

Expand Down Expand Up @@ -58,6 +60,13 @@ export interface UseScrollOptions extends ConfigurableWindow {
* @default 'auto'
*/
behavior?: MaybeRefOrGetter<ScrollBehavior>

/**
* On error callback
*
* Default log error to `console.error`
*/
onError?: (error: unknown) => void
}

/**
Expand Down Expand Up @@ -97,6 +106,7 @@ export function useScroll(
},
behavior = 'auto',
window = defaultWindow,
onError = (e) => { console.error(e) },
} = options

const internalX = ref(0)
Expand Down Expand Up @@ -169,10 +179,10 @@ export function useScroll(
if (!window)
return

const el = (
(target as Window).document
const el: HTMLElement = (
(target instanceof Window)
? (target as Window).document.documentElement
: (target as Document).documentElement ?? target
: (target as Document)?.documentElement ?? target
) as HTMLElement

const { display, flexDirection } = getComputedStyle(el)
Expand All @@ -181,7 +191,7 @@ export function useScroll(
directions.left = scrollLeft < internalX.value
directions.right = scrollLeft > internalX.value

const left = Math.abs(scrollLeft) <= 0 + (offset.left || 0)
const left = Math.abs(scrollLeft) <= (offset.left || 0)
const right = Math.abs(scrollLeft)
+ el.clientWidth >= el.scrollWidth
- (offset.right || 0)
Expand All @@ -206,7 +216,7 @@ export function useScroll(

directions.top = scrollTop < internalY.value
directions.bottom = scrollTop > internalY.value
const top = Math.abs(scrollTop) <= 0 + (offset.top || 0)
const top = Math.abs(scrollTop) <= (offset.top || 0)
const bottom = Math.abs(scrollTop)
+ el.clientHeight >= el.scrollHeight
- (offset.bottom || 0)
Expand Down Expand Up @@ -251,11 +261,15 @@ export function useScroll(
)

tryOnMounted(() => {
const _element = toValue(element)
if (!_element)
return

setArrivedState(_element)
try {
const _element = unrefElement(element as MaybeElementRef) // This can be a window/document
if (!_element)
return
setArrivedState(_element)
}
catch (e) {
onError(e)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the try cache here. Ideally we should make it safe. If it's an error that not possible to workaround, I'd prefer to expose a onError handler to users.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point 👍🏻

onError callback was added to packages/core/useScroll/index.ts

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. But the first thing is how would the error get triggered here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the root of the problem was highlighted by brendomaciel 👇🏻#3605 (comment)

})

useEventListener(
Expand Down