Make export default class the main file class in GDScript, add support for subclasses

[adamuso]

In addition to issue #59. This the proof of concept of my idea.

closes #59
closes #1

Main feature - export default is now main class, allow inner classes

1. First case

Creating a script that can be attached to a node in Godot

export default class MyNode extends Node2D {
  a: int = 10
}

Generating:

extends Node2D
class_name MyNode

var a: int = 10

Importing from TypeScript in another file

import MyNode from "./MyNode.gs"

const x = new MyNode();

2. Second case

Creating a script that can be attached to a node and have some helper classes

export default class MyNode extends Node2D {
  a: int = 10
}

export class HelperClass {
   b: string = "hello"
}

// Class that is not exported have no sense in gdscript, but in TS we can
// use this to limit someone from using that class through an import.
// This is important when we are writing a library for example
// Both cases with export and no export would generate same code for gdscript.
class NotExportedClass {
  c: boolean = false
}

Generating:

extends Node2D
class_name MyNode

var a: int = 10

class HelperClass:
  var b: string = "hello"

class NonExportedClass:
  var c: boolean = false

Importing from TypeScript in another file

import MyNode, { HelperClass, /* NonExportedClass - cannot be imported */ } from "./MyNode.gs"

const x = new MyNode();
const y = new HelperClass();

3. Third case

Creating a script that cannot be attached to a node in Godot, but have helper classes that can be imported to other scripts.

export class HelperClass {
   b: string = "hello"
}

class NotExportedClass {
  c: boolean = false
}

Generating:

class HelperClass:
  var b: string = "hello"

class NonExportedClass:
  var c: boolean = false

Importing from TypeScript in another file

import { HelperClass, /* NonExportedClass - cannot be imported */ } from "./MyNode.gs"

const y = new HelperClass();

Additional features

Extending inner classes

// src/BaseType.ts
export class InnerBaseType extends Node2D {

}

// src/MyClass.ts
import { InnerBaseType } from "./BaseType.ts"

export default class MyClass extends InnerBaseType {

}

Generates

# compiled/BaseType.gd
class InnerBaseType extends Node2D:
  pass

# compiled/MyClass.gd
extends "res://compiled/BaseType.gd".InnerBaseType
class_name MyClass

Anonymous export default class

export default class extends Node2D {

}

Generates

extends Node2D

# no class_name - which is valid for gdscript if you do not want to make class globally scoped

Extending anonymous classes

// src/BaseType.ts
export default class extends Node2D {

}

import BaseType from "./BaseType.ts"

// src/MyClass.ts
export default class MyClass extends BaseType {

}

Generates

# compiled/BaseType.gd
extends Node2D

# compiled/MyClass.gd

extends "res://compiled/BaseType.gd"
# this is a valid extends in gdscript, you can specify class name or class resource path in extends
class_name MyClass
var BaseType = load("res://compiled/BaseType.gd")

Inner classes super calls

export class BaseType extends Node2D {
  constructor(name: string) {
    super();
    print(name);
  }
}

export class MyClass extends BaseType {
  constructor() {
    super("MyName");
  }
}

Generates

class BaseType extends Node2D:
  _init(name: String).():
    print(name)

class MyCall extends BaseType:
  _init().("MyName"):
    pass

Enums no longer generate separate files

export enum MyEnum {
  A = 1,
  B = 2,
  C = 3
}

export default class MyClass extends Node2D {
  constructor() {
    super();
    print(MyEnum.B);
  }
}

Generates

extends Node2D
class_name MyClass

const MyEnum = {
  "A": 1,
  "B": 2,
  "C": 3,
}

func _ready():
  print(MyEnum.B)

[ksjogo]

Reading through the examples in the PR (which are very helpful, thank you), is also

export class X { 
}
export default X

supported/should it be?

[adamuso]

Reading through the examples in the PR (which are very helpful, thank you), is also

export class X { 
}
export default X

supported/should it be?

It should be, but I think this could go as another PR because this one is already very big :)

[johnfn]

Y'know, I didn't realize that you could import inner classes from other files, and I think that was one of my biggest confusions with why you wanted inner classes - they just seemed strictly worse :)

Now that I've clarified that to myself, I'm officially on board with this PR - the generated code just makes more sense than before! This PR also looks extremely thoughtful with all the cases you checked.

Two high level questions/suggestions about this before we merge it:

1. I think my biggest hold up with this right now is it's a little counterintuitive that one class must be marked as default in order for the generated code to be attached to a script. Although that's fairly standard practice, I'd say that only like 60% of the TS code that I see actually follows the "each file has a default" practice, and many classes may not have a default at all. Speaking for myself, I have a bunch of classes I've written in GD style TS that do not have a default export. My suggestion, to appeal both to people who do and do not use exports, is this:

• If a class only has a single export, ts2gd automatically considers that to be the main class.
• If a class has two classes and neither of them are default, ts2gd gives an error along the lines of "If you have two or more classes in a file, you must mark one of them class a default. This class will become the primary class, and all other classes in ${ filename } will be inner classes. If you want, it could choose one arbitrarily to be the main class (just so codegen doesn't totally fail) but this isn't as important.
• Otherwise, everything works as normal.

I think this would also help ease discoverability of this feature. I think my biggest worry is that new developers to ts2gd with TS experience would mark something as default and don't realize that that changes the codegen.

2. Although I support the principal of moving enums back into the same file, I'm a little leery. I believe one of the advantages I found while doing this was that if you had enums in the same file as the class, it was a lot easier to avoid circular references. For example, if A.ts defines Enum1, B.ts defines Enum2, and A and B make use of each other's enums, that will cause a circular reference, and that actually crashes the Godot compiler today. At least, I think it does. Would you mind verifying this just to be sure?

In general I'd also be keen to know how circular references pan out for multiple classes being defined in the same file. We should make sure it's not possible to cause that Godot error at all with this PR.

---

Anywho, TL;DR - this is fantastic, just want to make sure it's logical to our users and that it doesn't cause any circular errors.

[johnfn]

Here's a quick preliminary CR

[johnfn]

Would it actually be helpful to build a map of "class" to "is inner or normal" as a pass before codegen? And then we could just pass that into all the nodes in ParseState?

Mostly just asking out of curiosity, I can't really tell.

[johnfn]

I think one advantage of this is it'll help you reason about the whole "single class in a file is automatically default" case that I was suggesting without having to put that if statement everywhere... :)

[adamuso]

I don't know if "single class in a file is automatically default" is a good idea because what if someone wants a .gd only with one inner class? (asked the same question in my comment below).

Would it actually be helpful to build a map of "class" to "is inner or normal" as a pass before codegen? And then we could just pass that into all the nodes in ParseState?

About that I think we could improve it in the future but currently there is no need to do this. I don't want to make this PR even more complicated :)

[adamuso]

Now that I've clarified that to myself, I'm officially on board with this PR - the generated code just makes more sense than before! This PR also looks extremely thoughtful with all the cases you checked.

Thank you! I'm really happy that it makes sense not only for me :)

1. I think my biggest hold up with this right now is it's a little counterintuitive that one class must be marked as default in order for the generated code to be attached to a script. Although that's fairly standard practice, I'd say that only like 60% of the TS code that I see actually follows the "each file has a default" practice, and many classes may not have a default at all. Speaking for myself, I have a bunch of classes I've written in GD style TS that do not have a default export. My suggestion, to appeal both to people who do and do not use exports, is this:

I used default as marker for a main class in .gd file but actually if this is not desirable we can also give up on this idea and use some kind of decorator. We really just need a way to mark a class that should be treated as the main one, because otherwise we would need to explain why the codegen choosed class B instead of class A, but the user wants to have full control and manually choose class A.

• If a class only has a single export, ts2gd automatically considers that to be the main class.

What if you want to have a single class in file and you want it to be inner class?

• If a class has two classes and neither of them are default, ts2gd gives an error along the lines of "If you have two or more classes in a file, you must mark one of them class a default. This class will become the primary class, and all other classes in ${ filename } will be inner classes. If you want, it could choose one arbitrarily to be the main class (just so codegen doesn't totally fail) but this isn't as important.

We could do that but that will always force people to have a default class. That does not match Godot code which allows a .gd file without extends and class_name and that means you do not really want to use this gdscript code as an attachable script. I think the right solution is to create a nice readme or tutorial which clearly explains what a user needs to do to use specific Godot feature. Choosing a class automatically can be confusing so I think this shouldn't be the case. What do you think?

I think this would also help ease discoverability of this feature. I think my biggest worry is that new developers to ts2gd with TS experience would mark something as default and don't realize that that changes the codegen.

Instead of default we can use a decorator then. Although when converting TS to JS people understand that default changes codegen (it is a lot smaller difference but still), because for export class default X {} it actually generates exports.default = class X {} and not exports.X = class X {} like usual. Still we can just tell users that this is how it works and they won't be mad. I think discoverability should be achieved through some concise docs with references and the most important: examples. People usually learn on examples and less on the actual tool showing them errors, but that of course also really helps :)

2. Although I support the principal of moving enums back into the same file, I'm a little leery. I believe one of the advantages I found while doing this was that if you had enums in the same file as the class, it was a lot easier to avoid circular references. For example, if A.ts defines Enum1, B.ts defines Enum2, and A and B make use of each other's enums, that will cause a circular reference, and that actually crashes the Godot compiler today. At least, I think it does. Would you mind verifying this just to be sure?

In general I'd also be keen to know how circular references pan out for multiple classes being defined in the same file. We should make sure it's not possible to cause that Godot error at all with this PR.

I will research about the circular references and check if they are affecting genrated code. I've never had a problem with them in Godot so I need to get more info.

[ksjogo]

We could do that but that will always force people to have a default class. That does not match Godot code which allows a .gd file without extends and class_name and that means you do not really want to use this gdscript code as an attachable script. I think the right solution is to create a nice readme or tutorial which clearly explains what a user needs to do to use specific Godot feature. Choosing a class automatically can be confusing so I think this shouldn't be the case. What do you think?

Do we want to support library/util code without a class? Is that changing with this PR in some way?

Regarding circular references, in TS land you can run into them if you have code running on file load. If you have a circular reference on of the files will see the other's exports as undefined (without the types refelecting that possibility). I don't know how GD handles these though. TS solution is to not run anything on file load and instead wrap in a closure and then call that by some other file.

[adamuso]

Do we want to support library/util code without a class? Is that changing with this PR in some way?

@ksjogo I meant code like this below. Where you create .gd files in which you can have only utility code for example.

// src/utils.ts

// There is no class with `export default` what means that you cannot create a node with script attached from this file 

export class MyUtil1 { 
  utilFunc1() {
    return "a"
  }
}

export class MyUtil2 {
 utilFunc2() {
    return "b"
  }
}

# compiled/utils.gd
# notice no class_name or no extends, but inner classes still can be used by preloading 
# `preload("res://compiled/utils.gd").MyUtil1` in ohter `.gd` files

class MyUtil1:
  func utilFunc1():
    return "a"

class MyUtil2:
  func utilFunc2():
    return "b"

[ksjogo]

Can you check the generation of the dynamic definition files?

Are files now required to have that default export?
I have a game without default exports and the dynamic type generation for these wasn't triggered properly.
Should we warn if now default is found?

Additionally, when I change these to have default export, the interface name wrongly gets generated as e.g.
export default interface default { and not interface Actor (for things in _godot_defs/dynamic.

[adamuso]

Can you check the generation of the dynamic definition files?

Additionally, when I change these to have default export, the interface name wrongly gets generated as e.g.
export default interface default { and not interface Actor (for things in _godot_defs/dynamic.

Yes I will look into that later today.

Are files now required to have that default export?
I have a game without default exports and the dynamic type generation for these wasn't triggered properly.
Should we warn if now default is found?

Yes it is required if you want to generate a class with extends class_name in .gd file. We can add a warning but we definitely should not force that at least one class with default is a must. We can also change default to some decorator as I said in earlier comments.

[adamuso]

@ksjogo Fixed generating default as interface name in node paths in _godot_defs. :)

[adamuso]

I looked into circular references and basically Godot properly handles preload with circular reference (by showing an error), there is no program crash or something similar. That means Godot allows for users to create circular references and assumes that it is their mistake and not an error in engine itself.

I think ts2gd should have the same behavior: if someone creates circular reference in TS then this is theirs problem and ts2gd should not fix it for them (I refer to extracting enums to separate files). Of course this will force developers to be more aware of the code they write, but in the end when you writing TypeScript to JS you can also create circular references. In ts2gd it will be just more restricting. To fix such error in TS you can just move your enums to another file and this way we keep 1:1 .ts to .gd file conversion. Another way is that ts2gd can look for circular references and throw an error if they exist, but I don't how we could fully prevent them without splitting single .ts file to multiple .gd files.

[johnfn]

Just an update that I've been busy with work this week but I'll look into merging this soon :)

[ksjogo]

The .() at the end seems incorrect.

[adamuso]

This means that a base constructor is called (here are docs). If base constructor does not have arguments .() can be omitted. Why is it wrong? Does this code not compile or is wrongly generated?

[johnfn]

Yeah I know it looks weird but it really is valid GD syntax!

[johnfn]

I used default as marker for a main class in .gd file but actually if this is not desirable we can also give up on this idea and use some kind of decorator. We really just need a way to mark a class that should be treated as the main one, because otherwise we would need to explain why the codegen choosed class B instead of class A, but the user wants to have full control and manually choose class A.

Correct. We always need an explicit way to mark the class as main if the user has more than 1 class.

What if you want to have a single class in file and you want it to be inner class?

We could do that but that will always force people to have a default class. That does not match Godot code which allows a .gd file without extends and class_name and that means you do not really want to use this gdscript code as an attachable script. I think the right solution is to create a nice readme or tutorial which clearly explains what a user needs to do to use specific Godot feature. Choosing a class automatically can be confusing so I think this shouldn't be the case. What do you think?

I just can't understand why a user would ever want to have only inner classes. Is this a common pattern? I admit to being a bit of a gdscript novice... but I very rarely use inner classes at all, and I have never had a script with only inner classes. :P

The reason I bring this up is because I think that if ts2gd has to compromise between making an uncommon case annoying or a common case annoying, we should choose to make the uncommon case annoying. For instance, if people want to write a single inner class, but if it's only 1% of all people, I'd be totally fine with requiring a @inner decorator to generate that precise gdscript.

On the other hand, I feel that "one class per file" is extremely common, and we should make that case work without users even having to think about it. This is why I think people should be able to write export class Blah and have ts2gd do what we expect, even without default.

Still we can just tell users that this is how it works and they won't be mad. I think discoverability should be achieved through some concise docs with references and the most important: examples. People usually learn on examples and less on the actual tool showing them errors, but that of course also really helps :)

Yes, docs are good, but errors are better - and best is users that just write the right thing without even having to think about it. Yeah, some users are going to read the docs, but a lot of them are busy and don't have a lot of time to pore over everything we've written.

The most important thing, though?

My guess is that, for new users to ts2gd, writing a single class in a file is likely the very first thing they will do. Ideally they can do this - and have it work in Godot - without even looking up the documentation at all. :)

(I do admit that some aspects of ts2gd are tougher to guess without checking the docs... That is the ideal, however!)

(Although I do agree we need more example code :) I should honestly just publish some of my personal projects I've written with ts2gd...)

if someone creates circular reference in TS then this is theirs problem and ts2gd should not fix it for them

So the thing I think here is, in TS, these patterns would not cause circular reference errors. So, experienced TS programmers writing natural TS code and getting circular reference issues would actually be an unpleasant thing.

Also, on a personal note, I find circular references are extremely annoying. If there are easy ways to avoid them in generated code, we should try to do that :) If it's very hard, perhaps we could just warn on them. But I've definitely run into circular refs when writing gdscript and it's super annoying. I hope it gets fixed in gdscript 2...

EDIT: I checked and using load() instead of preload() avoids circular errors. So we can just codegen load - I don't see any disadvantage.

[johnfn]

All that said, here's my proposal:

Single class

TS:

export class Foo {} 

GD:

class_name Foo

Two classes, no default

TS:

export class Foo {}
export class Bar {} 

GD:

Error: Please mark Foo or Bar as the default (main) class. 

Two classes with default

TS:

export default class Foo {}
export class Bar {} 

GD:

class_name Foo

class Bar:
    pass

Single inner class

TS:

@inner // we can bikeshed on the name of this :) 
export class Foo {}

GD:

class Foo:
    pass

[adamuso]

Okay, this way works for me. Before I start reimplementing, one last question.

@johnfn Do we want to use export default to mark main class or use a decorator instead (for example @main)?

[adamuso]

I've changed the implementation to mostly match your proposal. I've added @inner decorator for marking inner classes and also added @main as an alternative for export default. I've also updated the readme and added more test cases.

[johnfn]

This is looking pretty good. With the writing tweaks I think we're good to go here.

I do have one suggestion, however. I feel like we're providing users a little too much choice between default and our @main and @inner decorators. I tend to prefer Python's ideology of "there should be one (and only one) way to do it."

With that in mind, how do you feel about removing the @main decorator, and requiring users to mark default? I think this would simplify our code and our users code. Our code, because we wouldn't have to worry about the weird edge cases when users use both @main and default, and our user's code because they won't get confused about which one they need to use.

I think we do need to preserve @inner for the single inner class special case, but I think it should be relegated to the realm of "use this extremely rarely for edge cases" rather than an equivalent option to export class Foo. Like, we should bury it in the README under "tips and tricks" or something. :)

I know this diff has been outstanding for a while, so if you want to just merge it now and fix that in a follow up, that makes sense to me :)

Thanks for the awesome diff. Can't wait to land this!

[adamuso]

I know this diff has been outstanding for a while, so if you want to just merge it now and fix that in a follow up, that makes sense to me :)

More changes to this PR right now will make it even more confusing. I agree that we should address removing @main in separate diff :).

I've done fixes that you requested in the review, there are two not resolved comments that you can look at. I think everything else is now ready to merged.

[adamuso]

@johnfn Hey, will you find some time to review and merge this? :)