-
Notifications
You must be signed in to change notification settings - Fork 178
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
enhancement: fixed typing and separted code #81
base: master
Are you sure you want to change the base?
Conversation
Thanks for your PR @esphung I've added a couple of comments.. Just wondering if we need to build anything now (since we've moved the files over to TS) before publishing to NPM? Or is that unnecessary? Also wondering if we should bump the major version on the package? I suppose you didn't make any breaking API changes -- so that might be overkill.. |
I was going to suggest adding an example app like what most standard RN libraries come with nowawdays; any create-react-native-library project. I wasn't sure how you would feel about that so I left it out. I suggest we test until the end of the month and then release, after that, I can continue working on it and just make small commits. PS: I can also add some unit test and GH actions. They are free for public repos |
Okey @esphung that's a good idea.. Let's test this PR/branch for a month (I'd like to test it with the latest version of Expo on iOS and Android) before merging into master and publishing a new version on NPM.
There's an example app that accompanies this library where we can add examples etc: https://github.com/bberak/react-native-game-engine-handbook I kept it separate to the core library because I didn't want to bloat the core engine with assets and other game-related resources
Because this was originally a pure-JS library, we bolted on types after (and we kept some of the entity-related types quite loose, because they kind of are 🤷♂️)..
Happy to hear more about this and how it would work |
Hey @esphung I was testing out this PR today on a demo app, and I found the TS version to be very choppy in terms of registering touch events.. It's weird, the framerate seems good, but it's almost like the touch events are not being registered in a smooth stream.. It's hard to explain, but I've created a branch that replicates this issue:
If you run the I tried to get some screen recordings of the issue: smooth-js.mp4choppy-ts.mp4 |
I'm going to close this PR and possibly try again from the ground up. I will make small PRs to a "mother" branch and we will do this thing. Last time (with this PR), I tried to do too much at once |
Hey @esphung Thanks for picking this up again.. Happy to talk any of this through with you.. I feel like this PR was definitely a step in the right direction -- so maybe just some tweaks here and there would've fixed the perf issues I mentioned.. but yeah, totally understand your approach of incremental changes, maybe the best place to start would be to keep the existing functions and classes and add some basic types inline with the source code (rather than in a separate d.ts file). Anyhow, thanks again for pushing this forward 🙏 |
📯 Description
Move to TypeScript ts(x) files so types, classes and code are up-to-date
📝 Summary of changes
🧪 How Has This Been Tested?