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
37 changes: 25 additions & 12 deletions packages/core/useScroll/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ 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 { unrefElement } from '../unrefElement'
import type { ConfigurableWindow } from '../_configurable'
import { defaultWindow } from '../_configurable'

Expand Down Expand Up @@ -58,6 +59,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 +105,7 @@ export function useScroll(
},
behavior = 'auto',
window = defaultWindow,
onError = (e) => { console.error(e) },
} = options

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

const el = (
(target as Window).document
? (target as Window).document.documentElement
: (target as Document).documentElement ?? target
) as HTMLElement
const el: Element = (
(target as Window)?.document?.documentElement
|| (target as Document)?.documentElement
|| unrefElement(target as HTMLElement | SVGElement)
) as Element

const { display, flexDirection } = getComputedStyle(el)

const scrollLeft = el.scrollLeft
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 +215,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 +260,15 @@ export function useScroll(
)

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

setArrivedState(_element)
try {
const _element = toValue(element)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const _element = toValue(element)
const _element = unrefElement(element)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could be the root of the problem 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. Sorry for the lack of explanation.

The problem occurs because, when using ref with components, its value is a component instance instead of the component's root element1. Naturally, since getComputedStyle doesn't exist on the instance object, an error is thrown.

Luckily, VueUse already have something to handle this, the unrefElement2 that I used in the suggestion.

Footnotes

  1. https://vuejs.org/guide/essentials/template-refs.html#ref-on-component

  2. https://vueuse.org/core/unrefElement/#unrefelement

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