-
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
Fixes Functions not in TS definition files #60 #61
base: master
Are you sure you want to change the base?
Fixes Functions not in TS definition files #60 #61
Conversation
Performs type checking for the renderer property, thus preventing anything other than JSX, or a reference to the React component class. Using the component class rather than a JSX element avoids issues with required props.
|
||
type GameEngineEntities = Record<string | number, GameEngineEntity>; | ||
|
||
export interface GameEngineProperties { | ||
systems?: any[]; | ||
entities?: {} | Promise<any>; | ||
renderer?: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @SimpleProgrammingAU - this all looks really good. Quick question, should this line be: renderer?: DefaultRenderer | any;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should probably be renderer: Renderer
and change the type definition for DefaultRenderer
to Renderer
to make it a bit more inclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.. I ask because of this change that you made: timer?: DefaultTimer | any;
- hence I was thinking we do the same for the renderer for the sake of consistency..
What exactly did you mean by:
and change the type definition for DefaultRenderer to Renderer to make it a bit more inclusive.
Are you saying that the Default
in DefaultRenderer
is redundant from a naming convention point of view? As in, the DefaultRenderer
function should simple adhere to a Renderer
function signature?
Lastly, since I'm not 100% across TS, would changing the name break anything from a backwards compatibility, tooling and intellisense (VS Code?) perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me have a play around with somethings myself because I'm not 100%, either! I'll get back to you tomorrow (I'm UTC+11) with either a new commit or more commentary =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SimpleProgrammingAU I'm curious where this ended up. Any updates on your findings?
@bberak what's the situation for this pull request? is something missing? I'm willing to participate |
Much appreciated @drplauska, I think we just need someone who is comfortable with TS to verify that this PR won't break backwards compatibility. Also, I think we need to rename the following types:
Then update the |
state: any; | ||
screen: ScaledSize; | ||
} | ||
export function DefaultRenderer(entities: any[], screen: ScaledSize, layout:LayoutRectangle): Component; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no such type LayoutRectangle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also no Component
|
||
interface GameEngineEntity { | ||
[key:string]: any; | ||
renderer?: JSX.Element | React.ComponentClass<any. any>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React.ComponentClass<any>
dispatch: (event:any) => void; | ||
start: () => void; | ||
stop: () => void; | ||
swap: ({}:any | Promise) => void | Promise<void> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
swap: ({}:any | Promise<any>) => void | Promise<void>
as Promise shall have 1 argument
stop: () => void; | ||
subscribe: (callback: () => void) => void; | ||
unsubscribe: (callback: () => void) => void; | ||
} | ||
|
||
interface TouchProcessorOptions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems like arguments for this interface may be optional because there are default values provided:
export default ({
triggerPressEventBefore = 200,
triggerLongPressEventAfter = 700,
moveThreshold = 0
}) => {
|
||
type GameEngineEntities = Record<string | number, GameEngineEntity>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this for? Doesn't look like it's used anywhere
running?: boolean; | ||
onEvent?: any; | ||
style?: StyleProp<ViewStyle>; | ||
children?: React.ReactNode; | ||
} | ||
|
||
export class GameEngine extends React.Component<GameEngineProperties> {} | ||
export class GameEngine extends React.Component<GameEngineProperties> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add
render: () => JSX.Element;
running?: boolean; | ||
onUpdate?: (args: GameLoopUpdateEventOptionType) => void; | ||
style?: StyleProp<ViewStyle>; | ||
children?: React.ReactNode; | ||
} | ||
|
||
export class GameLoop extends React.Component<GameLoopProperties> {} | ||
export class GameLoop extends React.Component<GameLoopProperties> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
render: () => JSX.Element;
here too
@bberak this doesn't look too bad, merging this shouldn't cause issues with backwards compatibility. |
Hey @drplauska, Really appreciate your reviews and comments 🙏 . I'm definitely not against rewriting the lib with typescript. I'd probably review the overall design of the components at the same time and perhaps opt for moving to something a bit more modern using hooks and functional components. I'm certainly open to any contributions if you're interested 👍. |
@bberak I'm gonna try and rewrite the lib asap |
Hi @drplauska, Perhaps we should just fix up the types in this PR first - to unblock people who might be relying on TS? I'll get in touch regarding a full rewrite (I got your email btw 👍) |
Hi @bberak, This seems like a very good lib and it'd be really great if it could be maintained. Was there any conclusion to the above conversation? |
Hey @shreykul I just need to sit down and work through this PR to make sure it's working as expected.. Seems like TS support is becoming more essential for adoption these days.. I'll try work through it over the weekend; and also maybe update the handbook repo to work with the latest version of Expo.. |
@bberak that'd be really great!! Can I try updating the handbook repo with typescript? I'm trying to learn typescript so it'd be a great project for me... |
Hey guys, I can help too if you want I'm not new to react native or Typescript but I am trying to use this library seriously. I ran into some things that I made a patch for, but could probably fix if you need the extra set of hands. Let me know if you need help. I can also just submit PRs. I wasn't sure if this repo was maintained because there hasn't been in update in a while |
Hey @esphung 👋, Thanks for offering to help. I'm happy to accept PRs (especially around updating/adding typing information). Also happy to chat about any other issues you ran into.. Cheers, Boris |
Can you inv me? I tried to make one but I couldn't because I want a member on GH |
As I am new to using
react-native-game-engine
I am not 100% that all of the correct functions have been exposed. Please review the changes I have made to ensure they match the expected use of the library.