Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proposed change in behavior for @JsProperty/@JsType with non-native Record types #6

Open
niloc132 opened this issue Jan 25, 2024 · 2 comments · May be fixed by #7
Open

Proposed change in behavior for @JsProperty/@JsType with non-native Record types #6

niloc132 opened this issue Jan 25, 2024 · 2 comments · May be fixed by #7

Comments

@niloc132
Copy link

https://openjdk.java.net/jeps/395 added Records to Java 16 as a finalized feature. In the course of implementing this for GWT 2, we've observed that it might make sense to slightly modify @JsProperty for non-native types in a way that doesn't automatically produce errors. It could also make sense to change how @JsType behaves on non-native records to automatically expose record components as properties rather than methods.

From the JEP:

A record class, and the components in its header, may be decorated with annotations. Any annotations on the record components are propagated to the automatically derived fields, methods, and constructor parameters, according to the set of applicable targets for the annotation. Type annotations on the types of record components are also propagated to the corresponding type uses in the automatically derived members.

This means that if @JsProperty is annotated on a record component, it will propagate to both the accessor method and the private final field. That is,

record Point(@JsProperty int x, @JsProperty int y) {}

would effectively become

record Point(@JsProperty int x, @JsProperty int y) {
  @JsProperty
  private final int x;
  @JsProperty
  private final int y;
  @JsProperty
  public int x() {
    return this.x;
  }
  @JsProperty
  public int y() {
    return this.y;
  }
}

This seems problematic for two reasons:

  • First, while jsinterop annotations are allowed on private members, they are not allowed on two members with the same name - x and x() would collide.
  • Second, x() and y() do not follow the expected name pattern.

Possible workarounds:

  • Changing the record component annotation from @JsProperty int x to @JsProperty("x") int x would technically resolve the naming issue, but with unnecessary verbosity, and still not correct the first problem
  • Even more verbosity could let a developer skip the annotation on the record component and instead add it to the accessor directly, writing:
record Point(int x, int y) {
  @JsProperty
  public int x() {
    return this.x;
  }
  @JsProperty
  public int y() {
    return this.y;
  }
}

This is usable, but arguably what the user clearly meant to begin with.

I propose instead that

  • @JsProperty on record component accessors not require that the accessor be named as a Bean getter, but reflecting the immutable nature of the record, only be named for the component.
  • @JsProperty on record instance fields are ignored if they are also present on the matching accessor method. This would allow a collision in naming, where the accessor should always "win" when exposing to JS.

Next, decorating a record with @JsType. This is unlikely to reflect the developer's intent, as by default the constructor and accessor methods would be treated as if decorated with their default jsinterop annotation. Instead, I suggest that the methods should be exported as properties. This makes the above example even simpler, offering a simple immutable record/struct to JS:

@JsType
public record Point(int x, int y) {}

As the constructor and accessors are implicit (but can be explicit, and in the case of the constructor, compact), if declared they could have their own jsinterop annotations present (@JsIgnore, for example), and if other methods are added, they would follow the standard rules that JsType follows - if public, they are annotated as expected. As records cannot have non-static fields added explicitly, this effectively means only constructors and methods will be able to be exposed in this way.

One observation: the compact constructor, in conjunction with these rules could simplify records with many components. Instead of the initial example where each component was annotated with @JsProperty, we could instead see

@JsType
public record Book(String title, int pageCount, List<String> authors) {
  @JsIgnore
  Book {
  }
}

It may also make sense to offer new behavior for native records, so a native Java record means something - perhaps a closure struct could make sense here? This would mean that invoking a native Java record's constructor would serve to create an object with the expected components, simple syntactic sugar to replace the interfaces with create() factory method that jsinterop-generator currently produces. This is a little more opinionated though (dealing with immutability, etc), so could be taken up separately.

@gkdn
Copy link
Member

gkdn commented Jan 26, 2024

Thanks for the write up. I think it generally makes sense though not sure what to do with native types. My quick take is JsType should mean the same thing everywhere and my there is a different annotation if we want to significantly change meaning (like JsFunction, JsEnum etc..).

@niloc132
Copy link
Author

niloc132 commented Mar 2, 2024

After some reflection, I think that @JsType(isNative=true) on an enum would be fairly similar in its wrongness, so in the same way that marking a Java enum as a native js type doesn't make sense, I think marking a Java record as native js type probably doesn't make sense.

I think I'd go further at this time and suggest that a closure @record is probably incompatible with Java record, even with a hypothetical @JsRecord annotation to paper over details:

  • Java records cannot extend from another record/class - contrast with closure's records, tooling like jsinterop-generator would need to extract extra interfaces to make this work.
  • Java records are always immutable (though https://bugs.openjdk.org/browse/JDK-8321133 could make this easier to handle in the future, but not in a js-compatible way) - it would not make sense to say "this java record represents a js record" and then permit the object to change.

Any read I have of it, there is no good analog in closure for an immutable record/struct type - even if you were to find a type already written as

/** @record */
function Book() {};
/** @const {string} */
Book.prototype.title;
/** @const {number} */
Book.prototype.pageCount;
/** @const {string[]} */
Book.prototype.authors;

you still have the "problem"/mismatch that the array in authors is mutable. Maybe we just punt on that, and acknowledge that Java developers will have to be aware that native records won't behave at all like Java records (in contrast to enums which have some compile time limitations, but no cases where values will change in surprising ways).

Given that, is there any way to permit native Java record types, without also applying runtime changes to the objects like calling freeze on them...?


The good news at least is that I don't think this needs to affect non-native records:

  • Either (non-native) @JsType'd Java records must follow the constraints above (so that they can compile at all in j2cl/gwt, without field/method naming issues), or
  • Java records must never be annotated with @JsType or compile errors will arise (without mandating that every record component get an extra annotation), and possibly
  • Java records must never be annotated with @JsType, but a never-native annotation such as @JsRecord be introduced to specifically handle this case and allow @JsType to never make exceptions for the above.

niloc132 added a commit to niloc132/jsinterop-annotations that referenced this issue Apr 7, 2024
Does not attempt to address native support - like enums, if this is to
be supported, it will likely require its own annotation and semantics.

Fixes google#6
@niloc132 niloc132 linked a pull request Apr 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants