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

Add Optional Parameter Destructuring for Generated TS Constructors #427

Closed
pbtura opened this issue Apr 4, 2018 · 9 comments
Closed

Add Optional Parameter Destructuring for Generated TS Constructors #427

pbtura opened this issue Apr 4, 2018 · 9 comments
Milestone

Comments

@pbtura
Copy link

pbtura commented Apr 4, 2018

When dealing with constructor methods with multiple parameters, using named parameters can help to make the code more readable and easier to refactor. While TypeScript does not directly support named parameters, it is possible to use destructuring to accomplish the same thing
(see: microsoft/TypeScript#467 ).

Currently, there is no way to use this pattern for classes generated by jsweet. Constructor generation is handled by the visitMethodDef() method of org.jsweet.transpiler.Java2TypeScriptTranslator. Since there are no PrinterAdapter methods used here that can be overridden, a custom adapter will not solve the issue. The only option would be a @replace in the java.

My proposal would be to allow the user to enable custom constructor generation via a method level annotation and/or a config rule ( i.e. generate a destructured constructor for methods with more than X arguments ) . For the actual TS generation I can see two possible approaches. The first would be to do the destructuring directly in the visitMethodDef() method and use the configuration to enable or disable it. The other option would be to add a method in the PrinterAdapter that can be overridden to customize constructor generation. The first method would be less work for the end user but would be less flexible.

What do you think? Is this something worth implementing, and if so, which approach would be better?

@lgrignon
Copy link
Collaborator

lgrignon commented Apr 4, 2018

Why do you need this feature? Are you using JSweet generated classes directly from TS code? Otherwise I don't see the point but I might miss it.
My natural answer would be, don't use many parameters for your methods, indeed not the most readable at call time but rather use parameter object https://refactoring.guru/introduce-parameter-object

Anyway, if you need it for sure, you can always provide a JSweetFactory which returns a subclass of Java2TypeScriptTranslator and overrides the visitMethodDef behavior. What do you think about it?

That being said, you seem to have a good understanding of the transpiler code so if you want to PR, I wil take a look at it.

Bye

@pbtura
Copy link
Author

pbtura commented Apr 4, 2018

Yes. I am using jsweet to generate the 'model' classes of a web application passed on an existing java backend. Those model classes are then used by angular controllers which are also written in TypeScript.
I agree the ideal solution would be to have less parameters, but sometimes you simply have to work with the java class as-is. That said, a parameter object is essentially what I am looking for. The idea of destructuring is to define an interface that specifies the names and types of the parameters. The constructor then takes a single parameter object that implements that interface.

If it is possible to do what I need with a factory, I will try that first. Do you happen to have any links to examples of how to do that?

@pbtura
Copy link
Author

pbtura commented Apr 5, 2018

I'm actually not even sure that using a factory is an option currently. It looks like there is a problem with the maven plugin ( I added a bug report here: lgrignon/jsweet-maven-plugin#40 ). There isn't much point in creating a custom factory if the maven plugin can't make use of it.

@lgrignon
Copy link
Collaborator

lgrignon commented Apr 6, 2018

I answered to your issue on the maven plugin :)
In which case would you trigger the destructuring? I perfectly understand how it works but could you provide a concrete use case with JSweet code and TS code please? I would guide you on how to contribute and / or write your extension

@pbtura
Copy link
Author

pbtura commented Apr 6, 2018

The Java class:

package example;

import java.util.Date;

import model.Bar;

public class Foo
{
	public Bar bar;
	public String greeting;
	public Date bDay;

	public Foo()
	{
	}
	
	public Foo( String greeting, Date bDay)
	{
		this.greeting = greeting;
		this.bDay = bDay;
	}

	public Foo(Bar bar, String greeting, Date bDay)
	{
		this.bar = bar;
		this.greeting = greeting;
		this.bDay = bDay;
	}
}

The default output would be:

/* Generated from Java with JSweet 2.2.0-SNAPSHOT - http://www.jsweet.org */
namespace example {
    export class Foo {
        public bar : model.Bar;

        public greeting : string;

        public bDay : string;

        public constructor(bar? : any, greeting? : any, bDay? : any) {
            if(((bar != null && bar instanceof <any>model.Bar) || bar === null) && ((typeof greeting === 'string') || greeting === null) && ((bDay != null && bDay instanceof <any>Date) || bDay === null)) {
                let __args = Array.prototype.slice.call(arguments);
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                (() => {
                    this.bar = bar;
                    this.greeting = greeting;
                    this.bDay = bDay;
                })();
            } else if(((typeof bar === 'string') || bar === null) && ((greeting != null && greeting instanceof <any>Date) || greeting === null) && bDay === undefined) {
                let __args = Array.prototype.slice.call(arguments);
                let greeting : any = __args[0];
                let bDay : any = __args[1];
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                (() => {
                    this.greeting = greeting;
                    this.bDay = bDay;
                })();
            } else if(bar === undefined && greeting === undefined && bDay === undefined) {
                let __args = Array.prototype.slice.call(arguments);
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
            } else throw new Error('invalid overload');
        }
    }
    Foo["__class"] = "example.Foo";

}

Now lets suppose the user set the config argument for the max arguments to 2. The expected output would be:

/* Generated from Java with JSweet 2.2.0-SNAPSHOT - http://www.jsweet.org */
namespace example {
  
    export interface FooArgs{
        bar?: any;

        greeting?: any;

        bDay?: any;
    }
  
    export class Foo {
        public bar : model.Bar;

        public greeting : string;

        public bDay : string;

        public constructor({bar, greeting, bDay}: FooArgs) {
            if(((bar != null && bar instanceof <any>model.Bar) || bar === null) && ((typeof greeting === 'string') || greeting === null) && ((bDay != null && bDay instanceof <any>Date) || bDay === null)) {
                let __args = Array.prototype.slice.call(arguments);
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                (() => {
                    this.bar = bar;
                    this.greeting = greeting;
                    this.bDay = bDay;
                })();
            } else if(((typeof bar === 'string') || bar === null) && ((greeting != null && greeting instanceof <any>Date) || greeting === null) && bDay === undefined) {
                let __args = Array.prototype.slice.call(arguments);
                let greeting : any = __args[0];
                let bDay : any = __args[1];
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                (() => {
                    this.greeting = greeting;
                    this.bDay = bDay;
                })();
            } else if(bar === undefined && greeting === undefined && bDay === undefined) {
                let __args = Array.prototype.slice.call(arguments);
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
                if(this.bar===undefined) this.bar = null;
                if(this.greeting===undefined) this.greeting = null;
                if(this.bDay===undefined) this.bDay = null;
            } else throw new Error('invalid overload');
        }
    }
    Foo["__class"] = "example.Foo";

}

and you would call the constructor with

new Foo({ bar: myBar, greeting:"hello world", bDay: today});

There is still the question of how to handle classes with multiple constructors. In the example above, I would expect to also be able to make a call like new Foo( "hello world", today); and have that be valid since the number of args for that constructor in the java class is <= than the max args config.

@lgrignon
Copy link
Collaborator

lgrignon commented Apr 8, 2018

I definitely don't think this is a good idea to integrate this as a standard JSweet option @renaudpawlak what's your opinion on it?

@pbtura you could give it a shot with your own JSweetFactory / translator but I think it would require a huge amount of work for a limite added value :)

Anyway, IMHO I would opt for plain JSweet DTO parameters:

package example;

@Interface
abstract class FooArgs {
  public Bar bar;
  public String greeting;
  public Date bDay;
}

import java.util.Date;

import model.Bar;

public class Foo
{
	public Bar bar;
	public String greeting;
	public Date bDay;

	public Foo()
	{
	}
	
	public Foo(FooArgs args)
	{
		this.bar = args.bar;
		this.greeting = args.greeting;
		this.bDay = args.bDay;
	}
}


// use it in another class
Foo foo = new Foo(new FooArgs() {{
  bar = ...;
  greeting = ...;
  bDay = ...;
}});

@renaudpawlak
Copy link
Contributor

Umm. I think I agree with Louis. It seems quite against basic Java intuition and standards to automatize such a refactoring.... And using a replace annotation would be quite ugly also I would say. So I think that Louis' solution is the better for now.

If you want conciseness, maybe the best way is to loose typing and pass an object to your constructor. Then you can use the jsweet.lang.Util.$map util function to pass the parameters... That's untyped but it works.

Please note that constructor overloading has limitations in JSweet.

@pbtura
Copy link
Author

pbtura commented Apr 9, 2018

Ok, thanks. Once the issues with using the factory in the maven plugin are ironed out, I'll experiment a bit and see what I can come up with. Once minor change that would be a HUGE help to the factory solution would be to extract the portion of the 'visitMethodDef()' method responsible for writing the constructor args into a separate method. Being able to override just that portion of the code would make things much simpler.

@pbtura
Copy link
Author

pbtura commented May 8, 2018

I was able to come up with something that is close to what I want without massive changes to jsweet. I added a pull request here: #434 . If you get some time, take a look and let me know what you think.

@lgrignon lgrignon added this to the 2.2.0 milestone May 9, 2018
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

No branches or pull requests

3 participants