Implement Type Guards for Scripting API

[TehCupcakes]

Is your feature request related to a problem? Please describe.

Layers and objects have various boolean properties to indicate what type of instance they are. In TypeScript, these are represented by different types. However, because these are just booleans and not type guard functions, TS cannot do narrowing on its own. That means that even after we run a condition to check isTileMap, intellisense only knows that we have an Asset, and properties on TileMap are not accessible without type errors.

Describe the solution you'd like

The API should include functions with type predicates in order to automatically narrow the type when an appropriate value is checked. Changing these to functions could be a breaking change to users (although easy to remediate). I believe the underlying JS implementation could use a getter to maintain backwards compatibility with property syntax (asset.isTileMap) while also allowing function syntax (asset.isTileMap()). However, TS would only allow one to be defined if they share the same name and does not support type predicates on getters, so it would have to be treated as a method in the TS definitions:

isTileMap(): this is TileMap;

Describe alternatives you've considered

You can work around the less specific type by manually asserting the type after the condition, using as. However, this is not type safe! There is no guarantee that you would get the type which you believe should be coming through. It leaves you susceptible to incorrect logical conditions, as well as not being adaptive for updates to the API which could break the expected type.

I implemented a type-safe workaround by creating the following abstract helper class with type guard functions. This adds some bloat to my extension which could instead be implemented within the scripting API for a better user experience. (In this case I created an abstract class to avoid extending Asset or Layer directly, and just make static calls. The real implementation would put these functions directly on the class prototype, and need not include an input argument.)

export abstract class Guard {
  // ASSET
  static isTileMap(a: Asset): a is TileMap {
    return a.isTileMap;
  }

  static isTileSet(a: Asset): a is Tileset {
    return a.isTileset;
  }

  // LAYER
  static isObjectLayer(l: Layer): l is ObjectGroup {
    return l.isObjectLayer;
  }
  static isGroupLayer(l: Layer): l is GroupLayer {
    return l.isGroupLayer;
  }
  static isImageLayer(l: Layer): l is ImageLayer {
    return l.isImageLayer;
  }
  static isTileLayer(l: Layer): l is TileLayer {
    return l.isTileLayer;
  }
}

[bjorn]

I believe the underlying JS implementation could use a getter to maintain backwards compatibility with property syntax (asset.isTileMap) while also allowing function syntax (asset.isTileMap())

The underlying implementation is in C++ and the TS file just tries to model the API. When I try to expose both a property and a function with the same name, the property appears to take precedence:

class EditableAsset : public EditableObject
{
    Q_OBJECT
    Q_PROPERTY(bool isTileMap READ isMap CONSTANT)
    // ...

public:
    // ...
    Q_INVOKABLE bool isTileMap() const { return isMap(); }

> tiled.activeAsset.isTileMap
$1 = true
> tiled.activeAsset.isTileMap()
TypeError: Property 'isTileMap' of object Tiled::EditableMap(0x1a32c30) is not a function

And actually, even in JS, it seems that once a function has been defined as a getter, it is no longer possible to call it as a function. So, I don't think we can make this work as a function in TS and as a boolean in JS. Instead, we need to choose between:

• Breaking compatibility with existing scripts. The required change might be small, but would break the script for older Tiled versions. It seems like keeping a script compatible with both is going to get ugly.
• Using a different name or scope for the function variants. Suggestions?
• Introducing functions along the lines of Asset.asTileMap() : TileMap | null. Though I realize that while these may be somewhat more convenient, they're not as nice as type predicates since they require using the return value.

[TehCupcakes]

I would favor the breaking change, to be honest. If it had a less desirable naming convention, then I would propose a different name for the new functions and mark the old ones as deprecated. But the isTileMap name is actually perfect and elegant; it's just unfortunate that it isn't a function. 😄

If you do decide to go with a different function, I think I would make asTileMap() use the type predicate, and just throw an exception if it's not actually a TileMap, instead of returning null. isTileMap is the "safe" way to check the type, so I think it's acceptable for asTileMap to be a bit more forceful... If the developer has determined this really must be a TileMap, an exception would be easier to troubleshoot in the case it doesn't match their expectation.

[eishiya]

As someone who's written a lot of scripts for the public, I am of course not in favour of a breaking change xP My impression was that the TS file existed primarily as a source to auto-generate the web docs rather than for actual TS use, and I write my scripts in JS, so I'd not even see any of the benefits of this change xP

So, I think I'd rather see Asset.asTileMap()/asTileset() as typed alternatives for people using TS. I feel isTileMap/isTileset are fairly semantic as booleans for JS usage, and I'd rather see those stay as-is.

[dogboydog]

I don't agree with making a breaking change for this use case, which on the whole is probably uncommon (bjorn and eishiya have more experience than me but I haven't seen any Tiled scripts written in Typescript until this issue)

[eishiya]

I haven't seen any Tiled scripts written in Typescript until this issue

Since TS has to be compiled to JS before it has to be used in Tiled, any finished scripts will necessarily be JS.

I've seen a few users ask questions over the years about installing the .ts file or report TS-specific problems such as this one. My impression is that most script writers use JS because that's the simpler option in most scenarios, but it's hard to know for sure unless they run into TS-specific problems or share their TS source code.

[dogboydog]

Yeah, I know Typescript compiles to JS, but looking at repositories for Tiled extensions I hadn't seen any Typescript source is what I meant

[TehCupcakes]

That's all fair. Ignoring the types for a second- the concern for me with adding a new function is that the purpose might not be clear, and having two properties for a similar thing adds clutter (for vanilla JS users too). But if one is just a boolean and the other is like a cast which throws on invalid type, I think that's an acceptable difference.

I fully expected an avoidance of breaking change, although the ease of migration makes it less bad in my mind. The counterpoint to consider is keeping the code clean at the same time.

TS is going to be a smaller percentage of users just because it takes more effort to set up, and if you aren't already familiar with it you probably won't be inclined to go through the learning curve. But its usage is also inhibited by some issues in the types (like this one) and lack of documentation on how to set up the compilation in such a way that works with the scripting engine. Sort of a chicken and egg situation.

[eishiya]

I imagine one day there will be a Tiled 2.0 that incorporates numerous breaking changes (including to the Map/Tileset file formats), and perhaps the scripting API for that can be designed with more knowledge of TS.

For now, I'd rather see that bit of "duplication". It even has a precedent in the API: we have Tileset.tile() and Tileset.findTile(), both of which return a Tile for the given ID, but one throws an error on invalid tile IDs, and the other returns null. In a vanilla JS scenario, just as tile/findTile are appropriate in different scenarios depending on how you intend to handle errors, so too would isTileMap/asTileMap(). Of course, the documentation could also mention that asTileMap and asTileset are intended primarily for TypeScript users, and that if(!asset.isTileMap) return; is sufficient in most other cases.