Skip to content
This repository has been archived by the owner on Jun 1, 2022. It is now read-only.

Return more readonly components #233

Merged
merged 4 commits into from
Aug 4, 2020
Merged

Return more readonly components #233

merged 4 commits into from
Aug 4, 2020

Conversation

sheepsteak
Copy link
Contributor

Some small changes to help with TypeScript development

  • Fix TypeScript types to show that functions can possibly return undefined and Readonly components
  • Return readonly/immutable components in getRemovedComponent as well
    • The rationale for this is that getComponent could already return a readonly removed component so this should too

if (!component) {
return;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No point going through queries if the component does not exist.

Copy link
Member

Choose a reason for hiding this comment

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

nice catch yep

@@ -52,7 +52,7 @@ export class Entity {
*/
getMutableComponent<C extends Component<any>>(
Component: ComponentConstructor<C>
): C;
): C | undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think this will be annoying? Should these functions throw an error (in development mode?) instead if the component does not exist?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's valuable to return undefined so you could check in your code if it exist or not instead of catching an error

@fernandojsg fernandojsg requested a review from robertlong August 2, 2020 21:08
@fernandojsg
Copy link
Member

Thanks!

@fernandojsg fernandojsg merged commit ce47c5d into ecsyjs:dev Aug 4, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants