Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .github/workflows/integration.yml

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm a little unsure where to put the internal hooks.

I initially considered the existing src/internal directory.
However, hooks would then depend on internal hooks, which in turn depend on usePreservedCallback from the hooks directory:
hooks/useDebouncedCallback → internal/useDebounce → hooks/usePreservedCallback

This doesn't create a circular dependency right now, but it seems like it could lead to one in the future. That's why I created a separate internal directory inside hooks.
However, this approach requires adding exclusions for that directory in parts of CI.
Would it be better to keep them in hooks/internal as they are now, or move them to the existing src/internal directory?
I'd appreciate your thoughts on this! 🙏

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For v1, we'd like to keep the implementation where it is (src/hooks/useDebounce, src/hooks/useThrottle) and keep using it internally. The public useDebounce / useThrottle will only be marked @deprecated in v1 (#537). In v2, we'll stop exporting them and move them under src/_internal/hooks, similar to es-toolkit's src/_internal.

Thank you for thinking this through with us and sharing your ideas! 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the clarification!
My understanding is that we'll keep useDebounce and useThrottle in their current locations (src/hooks/useDebounce and src/hooks/useThrottle), and hooks such as useThrottledCallback and useThrottledValue will continue referencing them as before.

For v1, the only change needed is to add @deprecated annotations to indicate that useDebounce and useThrottle will be removed from the public API in v2.

However, the current code references the implementations that were moved under _internal, and those changes have already been merged, so I'm not sure whether I've understood the plan correctly.

Is this something you plan to address as part of #537?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Once I have a clearer understanding of the scope, I think I can work on #537 tomorrow 👍

Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@ on:
- main
- v1-base

env:
INTERNAL_HOOKS_PATH: 'packages/*/src/hooks/_internal/*'

jobs:
quality:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -227,6 +230,9 @@ jobs:
- name: Check for JSDoc comments
run: |
for file in ${{ needs.check-changes.outputs.changed_files }}; do
case "$file" in
$INTERNAL_HOOKS_PATH) continue ;;
esac
dir_name=$(basename $(dirname "$file"))
base_name=$(basename $dir_name)
implementation="$(dirname "$file")/$base_name.ts"
Expand Down Expand Up @@ -279,6 +285,9 @@ jobs:
- name: Check JSDoc structure
run: |
for file in ${{ needs.check-changes.outputs.changed_files }}; do
case "$file" in
$INTERNAL_HOOKS_PATH) continue ;;
esac
directory=$(basename "$(dirname "$file")")
filename=$(basename $file)
filename="${filename%%.*}"
Expand Down Expand Up @@ -357,6 +366,10 @@ jobs:
continue
fi

case "$impl_dir" in
$INTERNAL_HOOKS_PATH) continue ;;
esac

impl_name=$(basename "$impl_dir")
impl_index="$impl_dir/index.ts"
impl_file=""
Expand Down
2 changes: 1 addition & 1 deletion .scripts/verifyDocsI18n.ts
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ try {
async directory =>
(
await fs.readdir(path.join(root, 'packages/react-simplikit/src', directory), { withFileTypes: true })
).filter(entry => entry.isDirectory()).length
).filter(entry => entry.isDirectory() && entry.name !== '_internal').length
)
)
).reduce((total, count) => total + count, 0);
Expand Down
10 changes: 6 additions & 4 deletions .vitepress/libs/legacyRedirects.mts
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,12 @@ export function collectLegacyRedirects(): RedirectPair[] {

function collectReferenceRedirects(): RedirectPair[] {
return ['hooks', 'components', 'utils'].flatMap(category =>
listDirectories(path.join(packageSourceRoot, category)).map(name => ({
from: `${RETIRED_MOBILE_PAGES.has(name) ? 'mobile' : 'core'}/${category}/${name}.html`,
to: `${category}/${name}.html`,
}))
listDirectories(path.join(packageSourceRoot, category))
.filter(name => name !== '_internal')
.map(name => ({
from: `${RETIRED_MOBILE_PAGES.has(name) ? 'mobile' : 'core'}/${category}/${name}.html`,
to: `${category}/${name}.html`,
}))
);
}

Expand Down
2 changes: 1 addition & 1 deletion .vitepress/shared.mts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ export const docsRoot = path.join(projectRoot, 'docs');
export function listDirectories(directory: string): string[] {
return fs
.readdirSync(directory, { withFileTypes: true })
.filter(entry => entry.isDirectory())
.filter(entry => entry.isDirectory() && entry.name !== '_internal')
.map(entry => entry.name);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
// Simplified version of https://github.com/toss/es-toolkit/blob/main/src/function/debounce.ts

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I noticed that we have tests for throttle, but none for debounce.

It looks like the debounce and throttle implementations were originally taken from es-toolkit and slightly adapted for simplikit.
Since we maintain this logic ourselves rather than delegating it to an external library, I think it would be good to add tests for debounce too :)

What do you think? I've left it as is for now, since it's outside the scope of this PR!

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed, it would be great to have tests for debounce too 👍

Would you like to open a follow-up PR for it? If you'd rather not, no worries. We can track it in an issue 🙌


// eslint-disable-next-line @typescript-eslint/no-explicit-any -- generic function constraint requires `any[]` due to parameter contravariance
export type DebouncedFunction<F extends (...args: any[]) => void> = {
(...args: Parameters<F>): void;
cancel: () => void;
};

type DebounceOptions = {
/**
* An optional array specifying whether the function should be invoked on the leading edge, trailing edge, or both.
* If `edges` includes "leading", the function will be invoked at the start of the delay period.
* If `edges` includes "trailing", the function will be invoked at the end of the delay period.
* If both "leading" and "trailing" are included, the function will be invoked at both the start and end of the delay period.
* @default ["leading", "trailing"]
*/
edges?: Array<'leading' | 'trailing'>;
};

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- generic function constraint requires `any[]` due to parameter contravariance
export function debounce<F extends (...args: any[]) => void>(
func: F,
debounceMs: number,
{ edges = ['leading', 'trailing'] }: DebounceOptions = {}
): DebouncedFunction<F> & { schedule: () => void } {
let pendingThis: ThisParameterType<F> | undefined = undefined;
let pendingArgs: Parameters<F> | null = null;

const leading = edges != null && edges.includes('leading');
const trailing = edges == null || edges.includes('trailing');

const invoke = () => {
if (pendingArgs !== null) {
func.apply(pendingThis, pendingArgs);
pendingThis = undefined;
pendingArgs = null;
}
};

const onTimerEnd = () => {
if (trailing) {
invoke();
}

cancel();
};

let timeoutId: ReturnType<typeof setTimeout> | null = null;

const schedule = () => {
if (timeoutId != null) {
clearTimeout(timeoutId);
}

timeoutId = setTimeout(() => {
timeoutId = null;

onTimerEnd();
}, debounceMs);
};

const cancelTimer = () => {
if (timeoutId !== null) {
clearTimeout(timeoutId);
timeoutId = null;
}
};

const cancel = () => {
cancelTimer();
pendingThis = undefined;
pendingArgs = null;
};

const debounced = function (this: ThisParameterType<F>, ...args: Parameters<F>) {
// eslint-disable-next-line @typescript-eslint/no-this-alias
pendingThis = this;
pendingArgs = args;

const isFirstCall = timeoutId == null;

schedule();

if (leading && isFirstCall) {
invoke();
}
};

debounced.schedule = schedule;
debounced.cancel = cancel;

return debounced;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { beforeEach, describe, expect, expectTypeOf, it, vi } from 'vitest';

import { renderHookSSR } from '../../../_internal/test-utils/renderHookSSR.tsx';

import { DebouncedFunction } from './debounce.ts';
import { useDebounce } from './useDebounce.ts';

describe('useDebounce', () => {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Looking at useDebounce on its own, I think several test cases are missing.
The current tests mainly verify how many times the callback is called.
It would be good to also test whether pending calls are cancelled when the delay or options change, and whether subsequent calls use the latest values when different values are passed in.

I haven't added these tests, since they're outside the scope of this PR.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed. Testing that pending calls are cancelled when the delay or options change, and that later calls use the latest values, would be valuable 👍

It could go into the same follow-up PR as the debounce tests!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks! The missing tests aren’t urgent, so I’ll add them after the major version is released.

beforeEach(() => {
vi.useFakeTimers();
});

it('is safe on server side rendering', () => {
const callback = vi.fn();
renderHookSSR.serverOnly(() => useDebounce(callback, 100));

expect(callback).not.toHaveBeenCalled();
});

it('should debounce the callback with default options', async () => {
const callback = vi.fn();
const { result } = await renderHookSSR(() => useDebounce(callback, 100));

result.current();
expect(callback).not.toBeCalled();

vi.advanceTimersByTime(50);
expect(callback).not.toBeCalled();

vi.advanceTimersByTime(50);
expect(callback).toBeCalledTimes(1);
});

it('should handle leading edge', async () => {
const callback = vi.fn();
const { result } = await renderHookSSR(() => useDebounce(callback, 100, { leading: true }));

result.current();
expect(callback).toBeCalledTimes(1);

result.current();
vi.advanceTimersByTime(50);
expect(callback).toBeCalledTimes(1);

vi.advanceTimersByTime(40);
expect(callback).toBeCalledTimes(1);

vi.advanceTimersByTime(10);
expect(callback).toBeCalledTimes(2);
});

it('should handle trailing edge', async () => {
const callback = vi.fn();
const { result } = await renderHookSSR(() => useDebounce(callback, 100, { trailing: true }));

result.current();
expect(callback).not.toBeCalled();

vi.advanceTimersByTime(50);
result.current();
expect(callback).not.toBeCalled();

vi.advanceTimersByTime(100);
expect(callback).toBeCalledTimes(1);
});

it('should handle both leading and trailing edges', async () => {
const callback = vi.fn();
const { result } = await renderHookSSR(() => useDebounce(callback, 100, { leading: true, trailing: true }));

result.current();
expect(callback).toBeCalledTimes(1);

result.current();
vi.advanceTimersByTime(150);
expect(callback).toBeCalledTimes(2);
});

it('should inference the callback type', async () => {
const callback = (value: string) => {
console.log('test::', value);
};

const { result } = renderHookSSR(() => useDebounce(callback, 100));

expectTypeOf(result.current).toEqualTypeOf<DebouncedFunction<typeof callback>>();
});

it('should cleanup on unmount', async () => {
const callback = vi.fn();
const { result, unmount } = await renderHookSSR(() => useDebounce(callback, 100));

result.current();
unmount();
vi.advanceTimersByTime(100);

expect(callback).not.toBeCalled();
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import { useEffect } from 'react';
import { useMemo } from 'react';

import { usePreservedCallback } from '../../usePreservedCallback/usePreservedCallback.ts';

import { debounce, DebouncedFunction } from './debounce.ts';

type DebounceOptions = {
leading?: boolean;
trailing?: boolean;
};

/**
* An internal React hook shared by debounce-related hooks.
* It debounces the provided callback and manages execution timing and cancellation of pending calls.
*
* With the default options, the last call runs after `debounceMs` milliseconds without another call.
* Pending calls are cancelled when the component unmounts or when `debounceMs`, `leading`,
* or `trailing` changes.
*
* `.cancel()` only cancels a pending callback; it does not stop work that has already started.
*/
export function useDebounce<F extends (...args: any[]) => unknown>(
callback: F,
debounceMs: number,
options: DebounceOptions = {}
): DebouncedFunction<F> {
const preservedCallback = usePreservedCallback(callback) as F;

const { leading = false, trailing = true } = options;

const edges = useMemo(() => {
const _edges: Array<'leading' | 'trailing'> = [];
if (leading) {
_edges.push('leading');
}

if (trailing) {
_edges.push('trailing');
}

return _edges;
}, [leading, trailing]);

const debounced = useMemo(() => {
return debounce<F>(preservedCallback, debounceMs, { edges });
}, [preservedCallback, debounceMs, edges]);

useEffect(
function cancelDebouncedOnUnmount() {
return () => {
debounced.cancel();
};
},
[debounced]
);

return debounced;
}
Loading
Loading