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

Fixes Functions not in TS definition files #60 #61

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SimpleProgrammingAU
Copy link

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.

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;
Copy link
Owner

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; ?

Copy link
Author

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.

Copy link
Owner

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?

Copy link
Author

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 =)

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?

@drplauska
Copy link

@bberak what's the situation for this pull request? is something missing? I'm willing to participate

@bberak
Copy link
Owner

bberak commented May 29, 2022

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:

  • DefaultTimer -> Timer
  • DefaultRenderer -> Renderer

Then update the GameEngineProperties interface to use the new type names (Timer, Renderer).

state: any;
screen: ScaledSize;
}
export function DefaultRenderer(entities: any[], screen: ScaledSize, layout:LayoutRectangle): Component;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no such type LayoutRectangle

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>;
Copy link

@drplauska drplauska May 31, 2022

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>
Copy link

@drplauska drplauska May 31, 2022

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 {

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>;

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> {

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> {

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

@drplauska
Copy link

drplauska commented May 31, 2022

@bberak this doesn't look too bad, merging this shouldn't cause issues with backwards compatibility.
What would you say about rewriting the lib with typescript? It wouldn't be necessary to maintain type declaration and the project would be easier to jump into for new people
I could work on that, but would probably need a little bit of consulting

@bberak
Copy link
Owner

bberak commented Jun 3, 2022

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 👍.

@drplauska
Copy link

@bberak I'm gonna try and rewrite the lib asap

@bberak
Copy link
Owner

bberak commented Jun 5, 2022

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 👍)

@shreykul
Copy link

shreykul commented Oct 8, 2023

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?

@bberak
Copy link
Owner

bberak commented Oct 9, 2023

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..

@shreykul
Copy link

shreykul commented Oct 9, 2023

@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...

@esphung
Copy link

esphung commented Jan 1, 2024

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

@bberak
Copy link
Owner

bberak commented Jan 4, 2024

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

@esphung
Copy link

esphung commented Jan 4, 2024

Can you inv me? I tried to make one but I couldn't because I want a member on GH

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.

6 participants