Object literal parameter gets incorrectly transpiled to optional parameters when object keys aren't strings

[jsayol]

The following code

function foo(bar) {
  console.log(foo['hello']);
}

foo({hello: 'world'});

Gets incorrectly transpiled to

foo(bar) {
  console.log(foo["hello"]);
}

foo(hello: "world");

Only when the object keys are strings ( foo({'hello': 'world'}) ) the transpiler generates the correct output

foo(bar) {
  console.log(foo["hello"]);
}

foo({"hello": "world"});

[jsayol]

This behavior seems to be intentional

https://github.com/angular/ts2dart/blob/6b1d8acd293ed3dbe5d523ba8d0e03cae2ac399b/lib/call.ts#L75-L89

but produces incorrect code nonetheless.

[mprobst]

Yeah, this is intentional. Dart has named parameters as a syntactical concept, where in TypeScript and ES6, they are just a convention of passing and destructuring an object literal. To allow idiomatic code in both worlds, ts2dart uses the convention of always translating an object literal that's passed as the last element into named parameter calls in Dart.

It's possible but not entirely trivial to handle this case correctly. Is this causing an actual problem in some context, or just an observation?

[jsayol]

Sure, I see how this can definitely be useful if keeping certain conventions while writing TS code, like you do in Angular.

I thought it'd be interesting to port ionic v2 to Dart as an experiment. It might be a naive attempt and it's possible I'll eventually end up hitting some roadblock but I want to give it a try anyway.
If I go on with this I will definitely encounter lots more issues that don't affect you guys when transpiling Angular. Would you be open making changes to ts2dart (obviously non-breaking) to adapt it to other uses, or am I better off forking?

For the issue at hand, this could easily be solved adding a command-line option to disable this behavior. Something like --handle-named-params=false, setting it to true by default. This approach could work well too for any other changes that might be necessary in the future.

[mprobst]

Sure, happy to take changes.

On this issue though note that we also translate the called site, i.e. functions with destructuring parameters, to named parameter Dart calls. So you'd have to disable both, and you might end up with substantially less idiomatic Dart code.

The alternative would be resolving what function is being called, and figuring out if that (after translation) takes named parameters or just an object literal. That needs type checking, see the facade_* for an idea of how that'd work.

I'm ok with either solution though.

[jsayol]

That's a good point, I didn't consider destructuring parameters although I did a quick test and I'm not sure they're being translated correctly.

TS:

function test({foo: bar}) {
  console.log(bar);
}

test({foo: 'hello'});

Generated incorrect Dart:

test({bar}) {
  console.log(bar);
}

test(foo: "hello");

Am I missing something?

Edit: removed quotes from the object key (test({'foo': 'hello'}) -> test({foo: 'hello'}))

Edit 2: this only seems to be a problem when using destructuring assignment in the parameters. This example, on the other hand, works:

TS:

function test({foo}) {
  console.log(foo);
}

test({foo: 'hello'});

Generated Dart:

test({foo}) {
  console.log(foo);
}

test(foo: "hello");

I might be getting off-topic here...

[jsayol]

On second thought (i.e. I stopped being lazy and actually gave it some thought) disabling this is not a good approach. It might work in some cases but if the same code also uses the object literals & destructuring parameters combo for optional named parameters, then we're in a bit of a pickle.

I'll look into the other option.