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

Implement getComponentOrThrow methods on Entity #257

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

liaujianjie
Copy link

@liaujianjie liaujianjie commented Aug 31, 2020

This PR adds getComponentOrThrow and getMutableComponentOrThrow methods on Entity so that we can throw a developer-friendly error message when we're trying to access a component that doesn't exist on an entity.

Typescript

Before

Typescript users currently need to either:

  1. Write a runtime check that discriminates the union for every single getComponent/getMutableComponent call.
    const aabb = myEntity.getComponent(AABBComp); // AABBComp | undefined
    if (!aabb) {
      throw new Error(`Component AABBComp is not on entity ${myEntity.id}.}`);
    }
    aabb; // AABBComp
  2. Or use non-null assertions (which defeats the purpose of using TS in strict null check mode)
    const aabb = myEntity.getComponent(AABBComp)!;
    //                                          ^ :-(

After

const aabb = myEntity.getComponentOrThrow(AABBComp); // AABBComp

Notes for reviewer

I'm not sure if creating separate methods is a good idea. Alternatively, we can add/modify the params for the existing component getter methods on Entity.

For example, adding a new param:

myEntity.getComponent(AABBComp, false, false);
//                                     ^^^^^ this param can be used to indicate
//                                           if the method should throw

I'll write the tests if this gets the green light.

@liaujianjie liaujianjie marked this pull request as ready for review September 1, 2020 03:45
@robertlong
Copy link
Member

I don't think we should throw when a component is not returned. A return type of Component | undefined is valid and likely what we want. You should guard against null components if you aren't sure if the entity has a component. If you are sure that component exists on an entity, you can use the null assertion operator !.

Aside from API semantics, I think including a branch that throws an exception in a method that is used a lot may have a negative impact on performance. I'm not certain about this, but I would think this is harder to optimize out and likely has a non-negligible impact on perf.

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

Successfully merging this pull request may close these issues.

2 participants