Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Next.js App Router, React v.19 deprecates forwardRef, other fixes #3

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ospira
Copy link
Contributor

@ospira ospira commented Dec 19, 2024

Hi, since I use this template and have been experimenting with it, I wanted to give back a little and submit this PR. I've summarized my changes below. Hope this is helpful and please leave as much feedback as you want, as I'm curious what you think. I also realize there is a chance you may want to split some aspects of this up from others, but hopefully in totality it makes sense and the diff is manageable. Thank you! :)

List/explanation of changes:

1. Update React deps to React v.19 (latest stable/production ready) and Next.js v.15 (latest stable/production ready).

  • Moved Next.js setup from pages router, to app router, which was first introduced in Next.js v.13 and now seems preferred/standard. Another motivation for this, besides it conforming to the latest Next.js standards, is that occasionally new people who use this template expecting the latest in Next.js routing conventions (app router) get confused by the pages setup (noticed some messages about this on the Discord).

2. React v.19 removes forwardRef so now we can just pass in ref prop to PhaserGame.tsx. This simplifies PhaserGame.tsx component signature significantly, and makes it easier to understand in the template.

  • I noticed the forwardedRef also had a nested ref of game (const game = useRef<Phaser.Game | null>(null!);). I maybe missing something regarding why this is necessary, but I removed this to further simplify PhaserGame.tsx, so now it just assigns a new scene (without a ref, as before) in the scene key, game basically stays the same as it is only written to at init. There was nothing wrong with the extra ref, it just seemed unnecessary and confusing in context of the template, when someone potentially new to Phaser<>React will want to understand how they work together. For that reason I opted for just the one ref. Apologies if this is wrong for some reason, in which case I'll be curious to learn about that!

3. Clean up some re-rendering and naming issues.

  • The simplified ref setup also showed that some of the effect dep arrays had unnecessary dependencies. One thing to note is that even [ref] is actually not needed in any of PhaserGame.tsx dep arrays, I just added them back because I believe some linters may complain. But I believe beyond that, they are not necessary. However the extra dep here was not needed, and was causing the useEffect block to run more often than necessary, although it didn't cause a huge problem re some quick perf tests, re-running the entire useEffect block because of that dep is best avoided, since I believe the intention is just to setup the EventEmitters and their listeners. This also simplifies the template code, and passes down a simpler setState function (setCanMoveSprite), which is more idiomatic React. This also removes this line: <PhaserGame ref={phaserRef} currentActiveScene={currentScene} /> which I personally found very confusing when first interacting with this template, as really this is just a way to set some state based off the EventEmitter, but reads like we are passing down the currentScene or its instance. Hopefully you agree the way I have done it here is more clear.

Other notes: When experimenting with this template, I did wrap the entire PhaserGame.tsx in memo(), and the code worked, but it did not influence performance by much if at all in my quick tests. The PhaserGame.tsx actually re-renders fairly frequently given the parent component hooks into a fair amount of game state/events (like setSpritePosition) but it seems the React reconciler handles this fine, furthermore given most of PhaserGame is just a container for the phaser game instance/canvas, wrapping in memo() seemed premature and also potentially confusing, but it was interesting. Also fun to note given how the new Next.js APIs, more of this template (really the non-phaser UI parts) are now statically generated (note the new dynamic API). Meaning, if the game takes really long to load for some reason or you disable javascript, at least you can see the DOM buttons! :D

Thanks for reading, lmk if I anything else I can do to help merge :)

@Zombieliu
Copy link

@ospira good job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants