fix(closes #527): Enable cow renaming #528

jeremyckahn · 2024-11-29T18:13:31Z

What this PR does

This PR fixes #527, a regression that prevented players from giving their cows custom names.

How this change can be validated

Successfully rename a cow (this is only possible with cows that were not received via trade).

Questions or concerns about this change

I have no idea how long this was broken for. 😭

vercel · 2024-11-29T18:13:35Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
farmhand	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 1, 2024 4:46pm

jeremyckahn · 2024-11-29T18:15:08Z

src/components/Inventory/Inventory.test.js

@@ -66,7 +66,7 @@ describe('Inventory Component', () => {
      const searchInput = screen.getByPlaceholderText('Search inventory...')
      fireEvent.change(searchInput, { target: { value: 'Carrot' } })

-      vitest.runAllTimers()
+      vitest.advanceTimersByTime(1000)


This improves a flakey test introduced by #526.

jeremyckahn · 2024-11-29T18:16:14Z

src/hooks/useMountState/useMountState.js

@@ -11,7 +11,7 @@ export const useMountState = () => {
    }
  }, [isMountedRef])

-  const isMounted = () => isMountedRef.current
+  const isMounted = useCallback(() => isMountedRef.current, [isMountedRef])


This fixes the root cause of #527.

Specifically, the fact that this wasn't wrapped in a useCallback meant that this useEffect would run on every render and reset the cow name (see line 137):

farmhand/src/components/CowCard/CowCard.js

Lines 128 to 138 in 56f72dc

useEffect(() => {

;(async () => {

const cowImage = await getCowImage(cow)

if (isMounted() === false) return

setCowImage(cowImage)

})()

setDisplayName(getCowDisplayName(cow, id, allowCustomPeerCowNames))

}, [cow, id, allowCustomPeerCowNames, isMounted])

using a ref in this code is kind of strange to me. if you like the ref use this isn't a big deal but i would have written this hook like:

const [isMounted, setIsMounted] = useState(false) useEffect(() => setIsMounted(true), []) return { isMounted }

in general though having isMounted checks is a code smell for me so it may be more
worthwhile to try to refactor all the places this hook is needed than to worry about rewriting it right now.

I'm glad you raised this because it made me realize that #526 introduced the inclusion of https://usehooks-ts.com/. This library provides an implementation of useIsMounted which effectively abstracts away the contents of this hook. Ironically the library implementation seems equivalent to what we have here, but since it's in a library we don't have to worry too much about the implementation details so long as it works (which it does).

I've switched to using the usehooks-ts implementation in 72d110b to eliminate the maintenance burden of our custom one.

in general though having isMounted checks is a code smell for me so it may be more
worthwhile to try to refactor all the places this hook is needed than to worry about rewriting it right now.

I generally agree and try to avoid using isMounted checks for this reason. In this case it was necessary for Cow image generation:

farmhand/src/components/CowCard/CowCard.js

Lines 129 to 135 in 72d110b

;(async () => {

const cowImage = await getCowImage(cow)

if (isMounted() === false) return

setCowImage(cowImage)

})()

In this part of the code, the Cow image is generated dynamically as an asynchronous operation. By the time the image generation is complete, it's possible that the component has unmounted which would mean setCowImage(cowImage) would be improperly setting state on an unmounted component. There are probably cleaner abstractions for this sort of thing, but this seemed like the simplest solution. I don't know that we'd want to repeat this pattern too much, but I think this is the only part of the code base that we do this right now so I think that this bit of isolated awkwardness is acceptable.

oh that's great we have a library version of this to use instead! also if it's only used for that one use case that's not a big deal at all. i did not look around for the uses

jeremyckahn · 2024-11-29T18:16:39Z

tsconfig.json

@@ -46,7 +46,7 @@
    // "noPropertyAccessFromIndexSignature": true,  /* Require undeclared properties from index signatures to use element accesses. */

    /* Module Resolution Options */
-    // "moduleResolution": "node",                  /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */
+    "moduleResolution": "node" /* Specify module resolution strategy: 'node' (Node.js) or 'classic' (TypeScript pre-1.6). */,


This seems to make type checking work better.

lstebner

leaving some thoughts but i'm also ok with a merge to get the bug fixed. i don't need to hold that up over semantics 😄

lstebner · 2024-12-01T00:38:26Z

src/components/CowCard/CowCard.test.js

+      userEvent.clear(nameInput)
+      userEvent.type(nameInput, customName)
+
+      await waitFor(() => {


i think you should be able to await the userEvent.type calls and avoid waitFor entirely

i think you should be able to await the userEvent.type calls and avoid waitFor entirely

It turns out that clear and type only return Promises in @testing-library/react v14. Farmhand currently uses v13, and upgrading to v14 breaks a number of other tests so it's not worth making that change in a narrowly-scope bugfix PR such as this.

However, it appears that the await waitFor(...) isn't even necessary here (I think it was for an earlier iteration), so I've removed it in 9601f41. Thanks for bringing this up helping to simplify this part of the code!

ah right, i forgot the switch to async was in later versions

lstebner · 2024-12-01T00:49:37Z

src/hooks/useMountState/useMountState.js

@@ -11,7 +11,7 @@ export const useMountState = () => {
    }
  }, [isMountedRef])

-  const isMounted = () => isMountedRef.current
+  const isMounted = useCallback(() => isMountedRef.current, [isMountedRef])


using a ref in this code is kind of strange to me. if you like the ref use this isn't a big deal but i would have written this hook like:

const [isMounted, setIsMounted] = useState(false) useEffect(() => setIsMounted(true), []) return { isMounted }

in general though having isMounted checks is a code smell for me so it may be more
worthwhile to try to refactor all the places this hook is needed than to worry about rewriting it right now.

test(#527): [wip] validate cow renaming

jeremyckahn

Thanks for the review @lstebner! The code is looking better with your feedback incorporated, as always. 🙂

I'll merge this now!

jeremyckahn · 2024-12-01T16:09:04Z

src/hooks/useMountState/useMountState.js

@@ -11,7 +11,7 @@ export const useMountState = () => {
    }
  }, [isMountedRef])

-  const isMounted = () => isMountedRef.current
+  const isMounted = useCallback(() => isMountedRef.current, [isMountedRef])


I'm glad you raised this because it made me realize that #526 introduced the inclusion of https://usehooks-ts.com/. This library provides an implementation of useIsMounted which effectively abstracts away the contents of this hook. Ironically the library implementation seems equivalent to what we have here, but since it's in a library we don't have to worry too much about the implementation details so long as it works (which it does).

I've switched to using the usehooks-ts implementation in 72d110b to eliminate the maintenance burden of our custom one.

in general though having isMounted checks is a code smell for me so it may be more
worthwhile to try to refactor all the places this hook is needed than to worry about rewriting it right now.

I generally agree and try to avoid using isMounted checks for this reason. In this case it was necessary for Cow image generation:

farmhand/src/components/CowCard/CowCard.js

Lines 129 to 135 in 72d110b

;(async () => {

const cowImage = await getCowImage(cow)

if (isMounted() === false) return

setCowImage(cowImage)

})()

In this part of the code, the Cow image is generated dynamically as an asynchronous operation. By the time the image generation is complete, it's possible that the component has unmounted which would mean setCowImage(cowImage) would be improperly setting state on an unmounted component. There are probably cleaner abstractions for this sort of thing, but this seemed like the simplest solution. I don't know that we'd want to repeat this pattern too much, but I think this is the only part of the code base that we do this right now so I think that this bit of isolated awkwardness is acceptable.

jeremyckahn · 2024-12-01T16:46:16Z

src/components/CowCard/CowCard.test.js

+      userEvent.clear(nameInput)
+      userEvent.type(nameInput, customName)
+
+      await waitFor(() => {


i think you should be able to await the userEvent.type calls and avoid waitFor entirely

It turns out that clear and type only return Promises in @testing-library/react v14. Farmhand currently uses v13, and upgrading to v14 breaks a number of other tests so it's not worth making that change in a narrowly-scope bugfix PR such as this.

However, it appears that the await waitFor(...) isn't even necessary here (I think it was for an earlier iteration), so I've removed it in 9601f41. Thanks for bringing this up helping to simplify this part of the code!

jeremyckahn · 2024-12-01T16:48:32Z

src/components/CowCard/CowCard.js

@@ -123,7 +123,7 @@ export const CowCard = (
    isCowOfferedForTradeByPeer && cowIdOfferedForTrade.length > 0
  )

-  const { isMounted } = useMountState()
+  const isMounted = useIsMounted()


As of 72d110b, this is now the solution to the root cause of #527.

jeremyckahn requested a review from lstebner November 29, 2024 18:13

jeremyckahn commented Nov 29, 2024

View reviewed changes

lstebner approved these changes Dec 1, 2024

View reviewed changes

jeremyckahn added 5 commits December 1, 2024 09:38

chore(tsconfig): set moduleResolution: node

feb24aa

fix(#527): enable cow renaming

28ba0d7

test(#527): [wip] validate cow renaming

fix(#526): improve flakey inventory test

b8fe71e

refactor(#527): use usehooks-ts implementation of isMounted

72d110b

fixup! fix(#527): enable cow renaming

9601f41

jeremyckahn force-pushed the bugfix/527__cow-renaming branch from 56f72dc to 9601f41 Compare December 1, 2024 16:44

vercel bot deployed to Preview December 1, 2024 16:46 View deployment

jeremyckahn commented Dec 1, 2024

View reviewed changes

jeremyckahn merged commit dd084d2 into develop Dec 1, 2024
5 checks passed

jeremyckahn deleted the bugfix/527__cow-renaming branch December 1, 2024 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(closes #527): Enable cow renaming #528

fix(closes #527): Enable cow renaming #528

jeremyckahn commented Nov 29, 2024

vercel bot commented Nov 29, 2024 •

edited

Loading

jeremyckahn Nov 29, 2024

jeremyckahn Nov 29, 2024 •

edited

Loading

lstebner Dec 1, 2024

jeremyckahn Dec 1, 2024 •

edited

Loading

lstebner Dec 1, 2024

jeremyckahn Nov 29, 2024

lstebner left a comment

lstebner Dec 1, 2024

jeremyckahn Dec 1, 2024

lstebner Dec 1, 2024

lstebner Dec 1, 2024

jeremyckahn left a comment

jeremyckahn Dec 1, 2024 •

edited

Loading

jeremyckahn Dec 1, 2024

jeremyckahn Dec 1, 2024

	useEffect(() => {
	;(async () => {
	const cowImage = await getCowImage(cow)

	if (isMounted() === false) return

	setCowImage(cowImage)
	})()

	setDisplayName(getCowDisplayName(cow, id, allowCustomPeerCowNames))
	}, [cow, id, allowCustomPeerCowNames, isMounted])

fix(closes #527): Enable cow renaming #528

fix(closes #527): Enable cow renaming #528

Conversation

jeremyckahn commented Nov 29, 2024

What this PR does

How this change can be validated

Questions or concerns about this change

vercel bot commented Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

jeremyckahn Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lstebner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremyckahn left a comment

Choose a reason for hiding this comment

jeremyckahn Dec 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vercel bot commented Nov 29, 2024 •

edited

Loading

jeremyckahn Nov 29, 2024 •

edited

Loading

jeremyckahn Dec 1, 2024 •

edited

Loading

jeremyckahn Dec 1, 2024 •

edited

Loading