- Notifications
You must be signed in to change notification settings - Fork1k
fix(site): updateuseClipboard to work better with effect logic#20183
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
7 commits Select commitHold shift + click to select a range
07c9967 refactor: change core clipboard hook API
Parkreinerb4c6648 fix: update most tests
Parkreiner206e215 fix: get all tests working
Parkreinerb0a9df9 chore: add new test cases
Parkreiner4bd341f fix: beef up test
Parkreiner37e0ce6 fix: beef up tests further
Parkreiner7f6e4e4 fix: update comments
ParkreinerFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -9,9 +9,11 @@ | ||
| * immediately pollutes the tests with false negatives. Even if something should | ||
| * fail, it won't. | ||
| */ | ||
| import { renderHook, screen } from "@testing-library/react"; | ||
| import { GlobalSnackbar } from "components/GlobalSnackbar/GlobalSnackbar"; | ||
| import { ThemeOverride } from "contexts/ThemeProvider"; | ||
| import { act } from "react"; | ||
| import themes, { DEFAULT_THEME } from "theme"; | ||
| import { | ||
| COPY_FAILED_MESSAGE, | ||
| @@ -115,8 +117,8 @@ function setupMockClipboard(isSecure: boolean): SetupMockClipboardResult { | ||
| }; | ||
| } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I have no idea why I made this generic originally | ||
| function renderUseClipboard(inputs?: UseClipboardInput) { | ||
| return renderHook<UseClipboardResult,UseClipboardInput>( | ||
| (props) => useClipboard(props), | ||
| { | ||
| initialProps: inputs, | ||
| @@ -188,9 +190,9 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { | ||
| const assertClipboardUpdateLifecycle = async ( | ||
| result: RenderResult, | ||
| textToCopy: string, | ||
| ): Promise<void> => { | ||
| await act(() => result.current.copyToClipboard(textToCopy)); | ||
| expect(result.current.showCopiedSuccess).toBe(true); | ||
| // Because of timing trickery, any timeouts for flipping the copy status | ||
| @@ -203,35 +205,35 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { | ||
| await act(() => jest.runAllTimersAsync()); | ||
| const clipboardText = getClipboardText(); | ||
| expect(clipboardText).toEqual(textToCopy); | ||
| }; | ||
| it("Copies the current text to the user's clipboard", async () => { | ||
| const textToCopy = "dogs"; | ||
| const { result } = renderUseClipboard(); | ||
| await assertClipboardUpdateLifecycle(result, textToCopy); | ||
| }); | ||
| it("Should indicate to components not to show successful copy after a set period of time", async () => { | ||
| const textToCopy = "cats"; | ||
| const { result } = renderUseClipboard(); | ||
| await assertClipboardUpdateLifecycle(result, textToCopy); | ||
| expect(result.current.showCopiedSuccess).toBe(false); | ||
| }); | ||
| it("Should notify the user of an error using the provided callback", async () => { | ||
| const textToCopy = "birds"; | ||
| const onError = jest.fn(); | ||
| const { result } = renderUseClipboard({ onError }); | ||
| setSimulateFailure(true); | ||
| await act(() => result.current.copyToClipboard(textToCopy)); | ||
| expect(onError).toBeCalled(); | ||
| }); | ||
| it("Should dispatch a new toast message to the global snackbar when errors happen while no error callback is provided to the hook", async () => { | ||
| const textToCopy = "crow"; | ||
| const { result } = renderUseClipboard(); | ||
| /** | ||
| * @todo Look into why deferring error-based state updates to the global | ||
| @@ -241,7 +243,7 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { | ||
| * flushed through the GlobalSnackbar component afterwards | ||
| */ | ||
| setSimulateFailure(true); | ||
| await act(() => result.current.copyToClipboard(textToCopy)); | ||
| const errorMessageNode = screen.queryByText(COPY_FAILED_MESSAGE); | ||
| expect(errorMessageNode).not.toBeNull(); | ||
| @@ -252,11 +254,91 @@ describe.each(secureContextValues)("useClipboard - secure: %j", (isSecure) => { | ||
| // Snackbar state transitions that you might get if the hook uses the | ||
| // default | ||
| const textToCopy = "hamster"; | ||
| const { result } = renderUseClipboard({ onError: jest.fn() }); | ||
| setSimulateFailure(true); | ||
| await act(() => result.current.copyToClipboard(textToCopy)); | ||
| expect(result.current.error).toBeInstanceOf(Error); | ||
| }); | ||
| it("Clears out existing errors if a new copy operation succeeds", async () => { | ||
| const text = "dummy-text"; | ||
| const { result } = renderUseClipboard(); | ||
| setSimulateFailure(true); | ||
| await act(() => result.current.copyToClipboard(text)); | ||
| expect(result.current.error).toBeInstanceOf(Error); | ||
| setSimulateFailure(false); | ||
| await assertClipboardUpdateLifecycle(result, text); | ||
| expect(result.current.error).toBeUndefined(); | ||
| }); | ||
| // This test case is really important to ensure that it's easy to plop this | ||
| // inside of useEffect calls without having to think about dependencies too | ||
| // much | ||
| it("Ensures that the copyToClipboard function always maintains a stable reference across all re-renders", async () => { | ||
| const initialOnError = jest.fn(); | ||
| const { result, rerender } = renderUseClipboard({ | ||
| onError: initialOnError, | ||
| clearErrorOnSuccess: true, | ||
| }); | ||
| const initialCopy = result.current.copyToClipboard; | ||
| // Re-render arbitrarily with no clipboard state transitions to make | ||
| // sure that a parent re-rendering doesn't break anything | ||
| rerender({ onError: initialOnError }); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| // Re-render with new onError prop and then swap back to simplify | ||
| // testing | ||
| rerender({ onError: jest.fn() }); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| rerender({ onError: initialOnError }); | ||
| // Re-render with a new clear value then swap back to simplify testing | ||
| rerender({ onError: initialOnError, clearErrorOnSuccess: false }); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| rerender({ onError: initialOnError, clearErrorOnSuccess: true }); | ||
| // Trigger a failed clipboard interaction | ||
| setSimulateFailure(true); | ||
| await act(() => result.current.copyToClipboard("dummy-text-2")); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| /** | ||
| * Trigger a successful clipboard interaction | ||
| * | ||
| * @todo For some reason, using the assertClipboardUpdateLifecycle | ||
| * helper triggers Jest errors with it thinking that values are being | ||
| * accessed after teardown, even though the problem doesn't exist for | ||
| * any other test case. | ||
| * | ||
| * It's not a huge deal, because we only need to inspect React after the | ||
| * interaction, instead of the full DOM, but for correctness, it would | ||
| * be nice if we could get this issue figured out. | ||
| */ | ||
| setSimulateFailure(false); | ||
| await act(() => result.current.copyToClipboard("dummy-text-2")); | ||
| expect(result.current.copyToClipboard).toBe(initialCopy); | ||
| }); | ||
| it("Always uses the most up-to-date onError prop", async () => { | ||
| const initialOnError = jest.fn(); | ||
| const { result, rerender } = renderUseClipboard({ | ||
| onError: initialOnError, | ||
| }); | ||
| setSimulateFailure(true); | ||
| const secondOnError = jest.fn(); | ||
| rerender({ onError: secondOnError }); | ||
| await act(() => result.current.copyToClipboard("dummy-text")); | ||
| expect(initialOnError).not.toHaveBeenCalled(); | ||
| expect(secondOnError).toHaveBeenCalledTimes(1); | ||
| expect(secondOnError).toHaveBeenCalledWith( | ||
| "Failed to copy text to clipboard", | ||
| ); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,18 @@ | ||
| import { displayError } from "components/GlobalSnackbar/utils"; | ||
| import { useCallback, useEffect, useRef, useState } from "react"; | ||
| import { useEffectEvent } from "./hookPolyfills"; | ||
| const CLIPBOARD_TIMEOUT_MS = 1_000; | ||
| export const COPY_FAILED_MESSAGE = "Failed to copy text to clipboard"; | ||
| export const HTTP_FALLBACK_DATA_ID = "http-fallback"; | ||
| export type UseClipboardInput = Readonly<{ | ||
| onError?: (errorMessage: string) => void; | ||
| clearErrorOnSuccess?: boolean; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I was going to say, as a gut reaction, I feel like I would expect this to clear the error on success by default, but actually it looks like we never use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Yeah, I can see that. I feel like we'lleventually need it, and the code is already set up to account for it in the tests, so I think merging it in now has the least friction. But I'll look into removing the error behavior in a bit MemberAuthor
| ||
| }>; | ||
| export type UseClipboardResult = Readonly<{ | ||
| copyToClipboard: (textToCopy: string) => Promise<void>; | ||
| error: Error | undefined; | ||
| /** | ||
| @@ -40,47 +36,56 @@ export type UseClipboardResult = Readonly<{ | ||
| showCopiedSuccess: boolean; | ||
| }>; | ||
| export const useClipboard = (input?: UseClipboardInput): UseClipboardResult => { | ||
| const { onError = displayError, clearErrorOnSuccess = true } = input ?? {}; | ||
| const [showCopiedSuccess, setShowCopiedSuccess] = useState(false); | ||
| const [error, setError] = useState<Error>(); | ||
| const timeoutIdRef = useRef<number | undefined>(undefined); | ||
| useEffect(() => { | ||
| const clearTimeoutOnUnmount = () => { | ||
| window.clearTimeout(timeoutIdRef.current); | ||
| }; | ||
| return clearTimeoutOnUnmount; | ||
| }, []); | ||
| const stableOnError = useEffectEvent(() => onError(COPY_FAILED_MESSAGE)); | ||
| const handleSuccessfulCopy = useEffectEvent(() => { | ||
| setShowCopiedSuccess(true); | ||
| if (clearErrorOnSuccess) { | ||
| setError(undefined); | ||
| } | ||
| timeoutIdRef.current = window.setTimeout(() => { | ||
| setShowCopiedSuccess(false); | ||
| }, CLIPBOARD_TIMEOUT_MS); | ||
| }); | ||
| const copyToClipboard = useCallback( | ||
| async (textToCopy: string) => { | ||
| try { | ||
| await window.navigator.clipboard.writeText(textToCopy); | ||
| handleSuccessfulCopy(); | ||
| } catch (err) { | ||
| const fallbackCopySuccessful = simulateClipboardWrite(textToCopy); | ||
| if (fallbackCopySuccessful) { | ||
| handleSuccessfulCopy(); | ||
| return; | ||
| } | ||
| const wrappedErr = new Error(COPY_FAILED_MESSAGE); | ||
| if (err instanceof Error) { | ||
| wrappedErr.stack = err.stack; | ||
| } | ||
| console.error(wrappedErr); | ||
| setError(wrappedErr); | ||
| stableOnError(); | ||
| } | ||
| }, | ||
| [stableOnError, handleSuccessfulCopy], | ||
| ); | ||
| return { showCopiedSuccess, error, copyToClipboard }; | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.