Skip to content

refactor(useRefEffect): Improve useRefEffect hook by adding generic types in docs and strict null check in implementation #273

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.

Sign up for GitHub

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

Merged
kimyouknow merged 9 commits into from
Mar 13, 2026
Merged

refactor(useRefEffect): Improve useRefEffect hook by adding generic types in docs and strict null check in implementation #273

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f910eb2
docs: add generic type
wo-o29 Sep 13, 2025
a8ecfc4
refactor: strict null check to effect callback
wo-o29 Sep 13, 2025
bfe5861
refactor: use loose null check for element in useRefEffect
wo-o29 Mar 3, 2026
8e54596
refactor: update type parameter from Element to RefElement in useRefE…
wo-o29 Mar 3, 2026
66ca52f
Merge branch 'main' into refactor/fix-userefeffect-null-check-and-gen…
wo-o29 Mar 3, 2026
3e34b87
docs: Add generic type support to useRefEffect interface
wo-o29 Mar 3, 2026
1f1d360
docs: Add generic type support to useRefEffect interface
wo-o29 Mar 3, 2026
2e7def8
Merge branch 'main' into refactor/fix-userefeffect-null-check-and-gen…
kimyouknow Mar 13, 2026
5e3bdaa
Merge branch 'main' into refactor/fix-userefeffect-null-check-and-gen…
kimyouknow Mar 13, 2026
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
10 changes: 5 additions & 5 deletions src/hooks/useRefEffect/ko/useRefEffect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
## 인터페이스

```ts
function useRefEffect(
callback: (element: Element) => CleanupCallback | void,
function useRefEffect<E extends HTMLElement>(
Copy link
Copy Markdown
Collaborator

@kimyouknow kimyouknow Mar 3, 2026

Choose a reason for hiding this comment

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

On the generic naming — I agree that the current source Element shadows the global DOM type, so a rename makes sense

However, our convention avoids abbreviations, so E might not be the best fit. What do you think about using a full name likeTargetElement or RefElement instead? Or if you have another idea, happy to hear it!

If we go with a rename, it'd be great to update both the source and docs together so they stay in sync.

Copy link
Copy Markdown
Contributor Author

@wo-o29 wo-o29 Mar 3, 2026

Choose a reason for hiding this comment

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

I used abbreviations based on the useLongPress and mouseDoubleClick documentation, but both hooks are now deprecated🤔

RefElement looks good :)

We will update both the source code and documentation!

callback: (element: E) => CleanupCallback | void,
deps: DependencyList
): (element: Element | null) => void;
): (element: E | null) => void;
```

### 파라미터

<Interface
required
name="callback"
type="(element: Element) => CleanupCallback | void"
type="(element: E) => CleanupCallback | void"
description="요소가 설정될 때 실행되는 콜백 함수예요. 이 함수는 정리 함수를 반환할 수 있어요."
/>

Expand All @@ -31,7 +31,7 @@ function useRefEffect(

<Interface
name=""
type="(element: Element | null) => void"
type="(element: E | null) => void"
description="요소를 설정하는 함수예요. 이 함수를 <code>ref</code> 속성에 전달하면, 요소가 변경될 때마다 <code>callback</code>이 호출돼요."
/>

Expand Down
10 changes: 5 additions & 5 deletions src/hooks/useRefEffect/useRefEffect.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,18 @@
## Interface

```ts
function useRefEffect(
callback: (element: Element) => CleanupCallback | void,
function useRefEffect<E extends HTMLElement>(
callback: (element: E) => CleanupCallback | void,
deps: DependencyList
): (element: Element | null) => void;
): (element: E | null) => void;
```

### Parameters

<Interface
required
name="callback"
type="(element: Element) => CleanupCallback | void"
type="(element: E) => CleanupCallback | void"
description="A callback function that is executed when the element is set. This function can return a cleanup function."
/>

Expand All @@ -31,7 +31,7 @@ function useRefEffect(

<Interface
name=""
type="(element: Element | null) => void"
type="(element: E | null) => void"
description="function to set the element. Pass this function to the <code>ref</code> attribute, and the <code>callback</code> will be called whenever the element changes."
/>

Expand Down
2 changes: 1 addition & 1 deletion src/hooks/useRefEffect/useRefEffect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export function useRefEffect<Element extends HTMLElement = HTMLElement>(
cleanupCallbackRef.current();
cleanupCallbackRef.current = () => {};

if (element == null) {
if (element === null) {
Copy link

Copilot AI Sep 13, 2025

Choose a reason for hiding this comment

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

The strict null check element === null will not catch undefined values, which could cause the callback to be executed with undefined. The original loose equality check == null was correct as it handles both null and undefined cases. This change could introduce bugs when the element is undefined.

Suggested change
if (element === null) {
if (element == null) {

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

@wo-o29 wo-o29 Sep 13, 2025

Choose a reason for hiding this comment

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

Since instance is of type T | null as in React CallbackRef Type, I don't think we need to check for undefined. What do you think?

Copy link
Copy Markdown
Collaborator

@kimyouknow kimyouknow Mar 3, 2026

Choose a reason for hiding this comment

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

You're right! — since the type is T | null, === null works correctly here.

That said, our codebase uses == null as the convention (see mergeRefs, mergeProps, debounce, etc.), so how about keep it consistent and revert to == null??

Copy link
Copy Markdown
Contributor Author

@wo-o29 wo-o29 Mar 3, 2026

Choose a reason for hiding this comment

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

I think it would be better to stick to the existing conventions!

return;
}

Expand Down
Loading