✨ [Feature] :added new hooks useHash, useHashHistory, useClipboard and useOrigin
#488
Conversation
|
|
|
||
| // Fast-forward time by 2000 milliseconds | ||
| await new Promise(resolve => setTimeout(resolve, 2000)); | ||
|
|
There was a problem hiding this comment.
I don't think that works when you are fast forwarding time in tests, you should use vitest.useFakeTimers() at the start of test, then vitest.advanceTimersByTime(delay) where you want to forward the time
| }); | ||
|
|
||
| // The isCopied state should be true immediately after copying | ||
| expect(result.current.isCopied).toBe(true); |
There was a problem hiding this comment.
This wouldn't work as clipboard.writeText is an async function and you are setting the value of isCopied in the then callback. So the value doesn't update immediately
| export function useClipboard({ timeout = 2000 }: { timeout?: number }) { | ||
| const [isCopied, setIsCopied] = useState<Boolean>(false) | ||
|
|
||
| const copyToClipboard = (value: string) => { | ||
| if (!value) return | ||
| if (typeof window === 'undefined' || !navigator.clipboard?.writeText) return | ||
|
|
||
| navigator.clipboard.writeText(value).then(() => { | ||
| setIsCopied(true) | ||
|
|
||
| setTimeout(() => { | ||
| setIsCopied(false) | ||
| }, timeout) | ||
| }) | ||
| } | ||
|
|
||
| return { isCopied, copyToClipboard } | ||
| } |
There was a problem hiding this comment.
I don't think we need another useClipboard while useCopyToClipboard exists, If you want the functionallity of setting isCopied and restting it after timeout you can do that by yourself outside of the hook. Or add that feature to the existing hook.
Don't create duplicate hooks as they tend to be confusing.
| originalHash = window.location.hash; | ||
| originalRemoveEventListener = window.removeEventListener; | ||
|
|
There was a problem hiding this comment.
I think you wanted to assign vi.fn() to window.removeEventListener, as you are testing later if removeEventListener was called or not. But it would fail as removeEventListner is not a spy.
| // Simulate a hashchange event with a new hash value | ||
| const newHash = "#newHash"; | ||
| window.location.hash = newHash; | ||
| const event = new Event('hashchange'); | ||
| window.dispatchEvent(event); |
There was a problem hiding this comment.
You might want to call it inside act(()=>{/* */})
| it('should return empty string before component has mounted', () => { | ||
| const { result } = renderHook(() => useOrigin()); | ||
|
|
||
| expect(result.current).toBe(''); | ||
| }); |
There was a problem hiding this comment.
As far as I know, renderHook mounts the component. so the test doesn't make sense. Also Origin is never "", for the sake of test the origin is supposed to be http://localhost:3000
| it('should return empty string before component has mounted', () => { | ||
| const { result } = renderHook(() => useOrigin()); | ||
|
|
||
| expect(result.current).toBe(''); | ||
| }); |
There was a problem hiding this comment.
As far as I know, renderHook mounts the component. so the test doesn't make sense. Also Origin is never "", for the sake of test the origin is supposed to be http://localhost:3000
| it('should return empty string if window.location.origin is not available', () => { | ||
| const originalLocation = window.location; | ||
| delete (window as any).location; | ||
| window.location = { origin: '' }as unknown as Location; | ||
|
|
||
| const { result } = renderHook(() => useOrigin()); | ||
|
|
||
| expect(result.current).toBe(''); |
There was a problem hiding this comment.
You want to test if window.location.origin is not available, you delete window.location but then you are setting origin to "" yourself. Then what is the point of doing the test?
| it('should return window.location.origin after component has mounted', async () => { | ||
| const originalLocation = window.location; | ||
| delete (window as any).location; | ||
| window.location = { origin: 'https://usehooks-ts.com' } as unknown as Location; | ||
|
|
||
| let result: any; | ||
| await act(async () => { | ||
| result = renderHook(() => useOrigin()).result; | ||
| }); | ||
|
|
||
| expect(result.current).toBe(''); | ||
|
|
||
| await act(async () => { | ||
| await new Promise(resolve => setTimeout(resolve, 0)); // Waiting for the next microtask | ||
| }); | ||
|
|
||
| expect(result.current).toBe('https://usehooks-ts.com'); | ||
|
|
||
| window.location = originalLocation; | ||
| }); |
There was a problem hiding this comment.
This whole test doesn't make any sense. Also I don't want to point out same things about fakeTimers as I have mentioned them previously.
| export const useOrigin = () => { | ||
| const [mounted, setMounted] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| setMounted(true); | ||
| }, []); | ||
|
|
||
| const origin = typeof window !== "undefined" && window.location.origin ? window.location.origin : ""; | ||
|
|
||
| if (!mounted) { | ||
| return ""; | ||
| } | ||
|
|
||
| return origin; |
There was a problem hiding this comment.
Why do you need a mounted when you are setting origin to "" based on typeof window !== "undefined"
There was a problem hiding this comment.
Got it thanks for feedback. Will fix it soon
| return () => { | ||
| window.removeEventListener("hashchange", handleHashChange); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
If I modify the code so that it renders anew every time the value of the hash changes, will this cause problems?
There was a problem hiding this comment.
Modifying the code to re-render on hash changes shouldn't inherently cause problems. It's done to ensure the component reacts to hash value changes accurately.
-key changes
four new hooks ✔️
docs ✔️
test cases✔️