ecsyjs / ecsy

Entity Component System for javascript
https://ecsyjs.github.io/ecsy/
MIT License
1.11k stars 115 forks source link

Implement `getComponentOrThrow` methods on `Entity` #257

Open liaujianjie opened 4 years ago

liaujianjie commented 4 years ago

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.

robertlong commented 4 years ago

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.