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

Proposal: code generation for dictionary types #6

Closed
tbenbrahim opened this Issue Jul 25, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@tbenbrahim

tbenbrahim commented Jul 25, 2017

There are many dictionaries that are used as arguments to methods. This is problematic because as currently generated, any type defined in a spec as a dictionary cannot be constructed in elemental .
Of course, there are workarounds, but they are ugly and not type-safe. Take for example the following example of Element::animate (which is not in Elemental2 yet):

JsPropertyMapOfAny keyframeAnimationOptions = JsPropertyMap.of();
keyframeAnimationOptions.set("duration",10_000);
keyframeAnimationOptions.set("iterations", Infinity);     
document.querySelector("div.clouds").animate(transitions,  Js.uncheckedCast(keyframeAnimationOptions));

The second parameter is a KeyframeAnimationOptions, but since I cannot construct it, I have to create it as a property map, hope I spell the property names correctly and uncheckedCast it. It is basically back to raw JavaScript.
What I would like to write instead is:

KeyframeAnimationOptions options=new KeyframeAnimationOptions();
options.setDuration(10_000);
options.setIterations(Infinity);
document.querySelector("div.clouds").animate(transitions,options);

The root problem is the @JsType annotation added to Dict types. Any class that does not have an equivalent class in JavaScript should instead be descended from JsObject (with the changes in Issue #5 ),
with getters and setters for the properties that call JsObject::set and JsObject::get, for example:

public class AnimationEffectTimingProperties extends JsObject {

    public void setDuration(double duration){
        set("duration", duration);
    }

    public void setDuration(String duration){
        set("duration", duration);
    }

    public AnimationEffectTimingReadOnly.DurationUnionType getDuration(){
        return get("duration");
    }

    public void setIterations(double iterations){
        set("iterations", iterations);
    }

    public double getIterations(){
        return get("iterations");
    }
// ... more properties here
}

This then allows me to write type safe code, with code completion.

For an example that is applicable to Elemental2, see GeoLocation::getCurrentPosition or GeoLocation:watchPosition, which take a Dictionary (per the spec) of type PositionOptions (GeoLocationPostionOptions in elemental), but which you will not be able to construct as currently defined.

@jDramaix

This comment has been minimized.

Member

jDramaix commented Jul 25, 2017

This is a known issue and we are evaluating the different ways to solve this.

  • implementing the interface methods using JsOverlay defender methods and add a factory method
  • provide a default implementation for each dictionary interfaces.
  • provide a fluent builder api.

That should be solved in the next version.

@gkdn

This comment has been minimized.

Member

gkdn commented Jul 25, 2017

@jDramaix with the factory, is there any use for @JsOverlay defender methods?

@tbenbrahim

This comment has been minimized.

tbenbrahim commented Jul 25, 2017

In my opinion, some of the ways suggested to solve the problem miss the point that these dictionaries are not @JsType. If there is not a window.Foo, there should not be a class or interface named Foo annotated with @JsType. Those dictonaries are all JavaScript Object and not some class defined in the DOM.

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Jul 25, 2017

If there is not a window.Foo, there should not be a class or interface named Foo annotated with @JsType.

Why? This is the way JsInterop is designed, whether you like it or not.

A @JsType(isNative=true) applied to an interface or with name="*" or name="?" has no real existence in JS and is just an alias to get strong typing in Java; just like Closure (and more generally jsdoc) has @typedef (1, 2).

No, I think the main issue is that JsInterop cannot directly map Closure record types, or WebIDL dictionaries, and the jsinterop-generator would have to find ways (as suggested by Julien above) to generate such code.

@tbenbrahim

This comment has been minimized.

tbenbrahim commented Jul 25, 2017

@tbroyer

I am working on my own JsInterop generator, that generates from IDL, this is what a dictionary looks like (as of a few minutes ago):, and it works quite well:

@JsType(name = "Object", namespace = JsPackage.GLOBAL, isNative = true)
public class AnimationEffectTimingProperties  {

    @JsConstructor
    public AnimationEffectTimingProperties(){
    }

    @JsOverlay
    public final void setDuration(double duration){
        Js.<Any>cast(this).asPropertyMap().set("duration", duration);
    }

    @JsOverlay
    public final void setDuration(String duration){
        Js.<Any>cast(this).asPropertyMap().set("duration", duration);
    }

    @JsOverlay
    public final AnimationEffectTimingReadOnly.DoubleOrStringUnionType getDuration(){
        return (AnimationEffectTimingReadOnly.DoubleOrStringUnionType) Js.<Any>cast(this).asPropertyMap().get("duration");
    }
    @JsOverlay
    public final void setIterations(double iterations){
        Js.<Any>cast(this).asPropertyMap().set("iterations", iterations);
    }

    @JsOverlay
    public final double getIterations(){
        return (double) Js.<Any>cast(this).asPropertyMap().get("iterations");
    }

    @JsOverlay
    public final void setDirection(String direction){
        Js.<Any>cast(this).asPropertyMap().set("direction", direction);
    }
}

The main points are:

  • EVery dictionary is @jstype(name="Object" ..., because it really is an Object, it is not a question of whether I like or not.
  • getters and setters for every property (now does not require any changes to JsObject)
  • a constructor
  • no surprises in the API, works like a Java object:
        KeyframeAnimationOptions logoAnimationOptions = new KeyframeAnimationOptions();
        logoAnimationOptions.setDuration(4_000);
        logoAnimationOptions.setIterations(Infinity);
        logoAnimationOptions.setDirection("alternate");
        logo.animate(logoTransitions, logoAnimationOptions);

demo: https://gwt-jelement.github.io/demo/index.html#element-animate

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Jul 25, 2017

Any reason you don't simply use @JsProperty rather than your @JsOverlay?

@JsProperty(name="duration") public double getDuration();
@JsProperty(name="duration") public void setDuration(double duration);

or even (less idiomatic of Java, but this is not the goal of JsInterop):

public double duration;

I think the reason jsinterop-generator uses interfaces is so that they could be implemented by any type (not necessarily a @JsType BTW), including ones that could implement several such interfaces (for cases where they share properties mostly, but not only), or simply anonymous types, which is probably why Julien didn't suggest changing from an interface to a class.

E.g. (not tested)

geolocation.getCurrentPosition(p -> null, p -> null, new GeolocationPositionOptions() {
  @Override public boolean isEnableHighAccuracy() { return true; }
  @Override public double getTimeout() { return 3000; }
  …
});

(note that because dictionary entries are all optional in the case of GeolocationPositionOptions, I believe they should be mapped to Boolean and Double instead of boolean and double, and they probably shouldn't have setters defined in the interface)
Defender methods would allow overriding only those you want to be "present" in the dictionary (others returning null, or possibly the default value; that way also you'd never forget to provide a required dictionary member); and a default implementation would likely be used the way you showed above (new KeyframeAnimationOptionsImpl()).

@jDramaix

This comment has been minimized.

Member

jDramaix commented Jul 25, 2017

@gkdn

This comment has been minimized.

Member

gkdn commented Jul 25, 2017

@jDramaix using overlays cause issues in multiple inheritance.

@jDramaix

This comment has been minimized.

Member

jDramaix commented Jul 26, 2017

Here is an example of what we could generate (just an idea, I didn't test if it works):

interface Foo {
  class Builder {
    private JsPropertyMap<Object> instance = JsPropertyMap.of();
    
    Builder bar(int bar) {
      instance.set("bar", bar);
      return this;
    }

    Builder baz(String baz) {
      instance.set("baz", baz);
      return this;
    }
    
    Foo build() {
      return Js.uncheckedCast(instance);
    }
  }

  static Foo.Builder builder() {
    return new Builder();
  } 

  @JsProperty
  int getBar();

  @JsProperty
  String getBaz();

  @JsProperty
  void setBar(int bar);

  @JsProperty
  void setBaz(String baz);
}

How to use it:

Foo myFoo = Foo.builder().bar(2).baz("foo").build();

The other alternative is just a factory method:

interface Foo {
  static Foo newInstance() {
    return Js.uncheckedCast(JsPropertyMap.of());
  } 

  @JsProperty
  int getBar();

  @JsProperty
  String getBaz();

  @JsProperty
  void setBar(int bar);

  @JsProperty
  void setBaz(String baz);
}

Usage:

Foo myFoo = Foo.newInstance();
myFoo.setBar(5);
myFoo.setBaz("foo");

The advantage of the first option is to provide a fluent API. It slightly more complex to put in place because we need to traverse the type hierarchy in order to find all JsProperties and create one method in the builder class for each JsProperty.

@tbroyer

This comment has been minimized.

Contributor

tbroyer commented Jul 26, 2017

Are the setters really needed in the interface? That would make it easier to implement the interface by other means; e.g.

new Foo() {
  @Override int getBar() { return 42; }
  @Override String getBaz() { return "towel"; }
}

With the second option, you could generate a MutableFoo interface then:

interface Foo {
  interface MutableFoo extends Foo {
    @JsProperty void setBar(int bar);
    @JsProperty void setBaz(String baz);
  }

  static MutableFoo newInstance() {
    return Js.uncheckedCast(JsPropertyMap.of());
  }

  @JsProperty int getBar();
  @JsProperty String getBaz();
}

that would be almost equivalent to providing a default implementation, except the default implementation here would actually be a JsPropertyMap.

@gkdn

This comment has been minimized.

Member

gkdn commented Jul 26, 2017

@tbroyer even without setter, there usually many getter methods that you need to implement even if you don't care. Also you instance with anonymous class is no longer JS Object but something more specific.

@jDramaix I think we should start with the simplest which is having a factory. Builder might be considered later maybe could be implemented outside of Elemental something similar to FreeBuilder..

jDramaix added a commit to google/jsinterop-generator that referenced this issue Sep 21, 2017

Add static factory method for dictionary types.
Fix wrong conversion of structural type used in an typedef. Those types were converted to a java class instead of java interface.

See google/elemental2#6

PiperOrigin-RevId: 169399254

jDramaix added a commit that referenced this issue Sep 21, 2017

Add static factory method for dictionary types.
Fix wrong conversion of structural type used in an typedef. Those types were converted to a java class instead of java interface.

See #6

PiperOrigin-RevId: 169399254
@jDramaix

This comment has been minimized.

Member

jDramaix commented Nov 12, 2017

We now provide staticfactory for dictionary types

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment