google / jsinterop-generator

Generates Java annotated with JsInterop from JavaScript extern sources
Apache License 2.0
75 stars 24 forks source link

`@record` types with no members have no create() method #51

Open niloc132 opened 1 year ago

niloc132 commented 1 year ago

Steps to reproduce, create a record type with no members.

/**
 * @record
 */
function MyRecord() {}

Expected, a Java interface with a static create() method. Actual output is a Java interface with no contents.

--

While this might seem like a weird edge case, there are at least two ways that this comes up. First, when tsickle generates externs from typescript where a type is re-exported in another file, tsickle will generate a new type that extends the existing type. Second, a record type that extends from two other record types, but doesn't contribute new fields.

/**
 * @record
 */
function ParentRecordA() {}

/**
 * @type {string}
 */
ParentRecordA.prototype.name;

/**
 * @record
 */
function ParentRecordB() {}

/**
 * @type {number}
 */
ParentRecordB.prototype.value;

/**
 * @record
 * @extends {ParentRecordA}
 * @extends {ParentRecordB}
 */
function ChildRecord() {}

Expected Java output,

@JsType(isNative = true, namespace = JsPackage.GLOBAL)
public interface ChildRecord extends ParentRecordA, ParentRecordB {
  @JsOverlay
  static ChildRecord create() {
    return Js.uncheckedCast(JsPropertyMap.of());
  }
}

Actual Java output

@JsType(isNative = true, namespace = JsPackage.GLOBAL)
public interface ChildRecord extends ParentRecordA, ParentRecordB { }

An easy workaround is to just create an instance of a supertype, or just a plain JsObject.create(null) or JsPropertyMap.of(), then wrap in a cast as the usual implementation does. But this isn't expected to be the correct way, since all other records get a generated factory method.

niloc132 commented 1 year ago

Test demonstrating the issue https://github.com/google/jsinterop-generator/compare/master...Vertispan:jsinterop-generator:empty-record-type-factory-method

gkdn commented 1 year ago

Hmm isDictionaryType decision seems straightforward but looks like TypeReference abstraction doesn't resolve fields so we currently cannot tell if parent has any fields.

niloc132 commented 1 year ago

Does it matter if the parent has fields or not? Even if a record with no supertype has no fields, if the user must create an instance to use some api, it makes more sense to call MyRecord.create() than to manually write out the Js.cast(JsPropertyMap.of()) call - or, if this doesn't make sense, it seems reasonable to remove all such create() method, as the user can just as easily make this call in all places where a record is created?

gkdn commented 1 year ago

Even if a record with no supertype has no fields, if the user must create an instance to use some api, it makes more sense to call MyRecord.create() than to manually write out the Js.cast(JsPropertyMap.of()) call

My recollection is; when we emit create methods unconditionally it had some unintended effect (though I don't remember what it was).