adrfoong / t3

MIT License
0 stars 1 forks source link

Force rerender with hooks into game model #1

Open adrfoong opened 3 years ago

adrfoong commented 3 years ago

https://github.com/adrfoong/t3/blob/a1874cd50b043b1b6ae4e28796f4c3c62e953f1d/src/Board.tsx#L98-L125

The game board UI is based on the game model, which never changes and thus never causes the components to rerender. In order to solve this issue, I went with an event-based approach by allowing the UI to hook onto various model change events, allowing it to rerender.

While I don't think this is a bad approach, I wonder what people think about this. I really want the game model drive the UI, but this approach seems to go against the "never mutate props" convention.

karkipy commented 3 years ago

you could implement this into a context which will make it look more cleaner

4shub commented 3 years ago

Your game state should be controlled by react and not as an independent class. Forcing setStates are an antipattern.

What I would recommend instead of a traditional GameManager is build a custom react hook that returns the functions you need to control the game.

More info: https://reactjs.org/docs/hooks-custom.html

Making your game state a react controlled element will allow you to render your content a specific amount of times when necessary

Hope this helps and let me know if you need a code sample to better demonstrate a custom hook.

dtinth commented 3 years ago

Came here from https://dev.to/adrfoong/public-code-review-4lh4

Yup, passing mutable values around in React can lead to situations where it is hard to optimize the re-rendering. My recommendation is that you can keep the Game model as it is but see if you can shield the rest of the UI from accessing it, e.g.

export const Game = ({ game }) => {
  const gameState = useGameState(game)

The useGameState custom hook takes a Game which is mutable and returns an immutable object representing the game state. You can then pass this value around.

You can also keep passing game into child components but I think it's best to treat it as a reference and do not use it to render components, instead passing the game prop to custom hooks that returns an immutable version of it (and take care of subscription).

export const Board = ({ game, active }: BoardProps) => {
  const cells = useGameCells(game)
const History = ({ game }) => {
  const history = useGameHistory(game)

With this, the useGameCells and useGameHistory can use whatever technique is required (including forcing a re-render), because it is now an implementation detail and does not affect the rest of your React app.

adrfoong commented 3 years ago

@karkipy Thanks, there are definitely things I can clean up here.

@4shub I didn't even think of using custom hooks, and I think it's a good idea as @dtinth has explained (and maybe what you're also going for?). However, I don't necessarily think that I have to let React control my game state. With the current game manager, I can play the game without relying on a UI if I choose to. I believe this is more of an pubsub-based paradigm, which I think is what libraries like mobx does.

@dtinth Thanks for explaining your reasoning behind creating the custom hook. I think that's a great way to compartmentalize the game state as you suggested. What are other techniques that you think will be viable here besides forcing the re-render?

dtinth commented 3 years ago

What are other techniques that you think will be viable here besides forcing the re-render?

Instead of forcing a rerender, like setState({}), you can copy data from the game into React state, like useGameHistory can setGameHistory([...game.history]) each time the history changes, assuming each individual entries in the history is not mutated.