rbxts-flamework / core

Flamework is an extensible game framework. It requires typescript and offers many useful features.
MIT License
101 stars 8 forks source link

Incorrect typing for Components.getComponent() in NPM package #34

Closed mwalk-dev closed 2 years ago

mwalk-dev commented 2 years ago

The actual signature is getComponent<T>(instance: Instance, componentSpecifier?: Constructor<T> | string) but the index.d.ts file in the NPM package does not allow for string specifiers to be passed in.

Fireboltofdeath commented 2 years ago

This is by design. Flamework uses string IDs internally but users are expected to pass a constructor or use the generic syntax since the string syntax does not provide sufficient typing. Do you have a reason for wanting to use strings instead of passing in the component constructor?

mwalk-dev commented 2 years ago

I have shared Components and I'm trying to create a RemoteFunction that can be invoked from any of these Components in order to retrieve and operate on the corresponding Component instance on the other side of the network boundary. So, for instance, a client-side Tool can make a request of the server-side Tool's Component to do something that affects game state - fire a gun or cast a spell or whatever. In order to have a single RemoteFunction for this, the function parameters need to hold all information required to find the matching Component on the other side of the boundary. I believe the constructor can't be used for this, because it can't be serialized.

For the example I mentioned before (after modifying index.d.ts), the server-side callback looks something like:

Functions.requestAction.setCallback((player, tool, specifier) => {
   // ValidatedComponent is my abstract base component
   const cmp = this.components.getComponent(tool, specifier) as ValidatedComponent | undefined; // This is messy, I agree
   if (!cmp) {
      return false;
   }
   if (cmp.validateAndExecute()) {
      // Broadcast successful activation to players
   }
});

If you think this is something that Flamework's public API shouldn't support then I'll take a different approach, but this seemed more straightforward than the alternatives I've considered - having one RemoteFunction per Component class, or maintaining a separate explicit mapping of serializable identifier to constructor.

Fireboltofdeath commented 2 years ago

Flamework does expose id->constructor mapping via Reflect.idToObj, which is what getComponent uses internally when Flamework passes a string instead of a constructor. I generally try not to expose the string ID apis publicly and reserve them solely for Flamework/internal use as they aren't type-safe and you can easily pass incorrect values to them.

mwalk-dev commented 2 years ago

It definitely makes sense to prefer type safety wherever possible. I still tend to think this is worth exposing for cases like serializing/deserializing a component reference. There are certainly other ways I can handle that, but all of them at some point basically involve a runtime test of the assumption that given X indicator being sent over the network, I can find a component of type Y on some Instance. I don't love having to cast to ValidatedComponent in my example, but it strikes me as a necessary evil given the use case.

Fireboltofdeath commented 2 years ago

I'll consider exposing the string APIs, but for now you can just use Reflect.idToObj.get(your_id) and pass that into getComponent instead of the ID.

Fireboltofdeath commented 2 years ago

https://github.com/rbxts-flamework/components/commit/f3b3626fffa183a76c964d706f2643706bf619c0 https://github.com/rbxts-flamework/transformer/commit/21029c3b6b5f7a44abf5960cbf37bd611efe6f2e