Skip to content

ObjectBindingPatterns Implementation#376

Merged
tomblind merged 16 commits intoTypeScriptToLua:masterfrom
hazzard993:objectbindingpatterns
Feb 1, 2019
Merged

ObjectBindingPatterns Implementation#376
tomblind merged 16 commits intoTypeScriptToLua:masterfrom
hazzard993:objectbindingpatterns

Conversation

@hazzard993
Copy link
Copy Markdown
Contributor

@hazzard993 hazzard993 commented Jan 30, 2019

I've implemented Object Binding Patterns.

They can be used in methods, functions and declarations.

function test({x, y}) {
    print(x, y);
}
let {x, y} = {x: 0, y: 1};

test({x: 0, y: 1});     // Prints 0, 1
print(x, y);            // Prints 0, 1

They can have default values / initializers.

let {x = 5, y} = {y: 5};
print(x, y);            // Prints 5, 5

They can be nested.

let {x: {x, y}, z} = {x: {x: 0, y: 1}, z: 2};
print(x, y, z);         // Prints 0, 1, 2

They can have aliases.

let {x: foo, y: bar} = {x: 0, y: 1};
print(foo, bar);        // Prints 0, 1

They can also be exported.

export let {x, y} = {x: 0, y: 1};

I removed the throw from transformVariableDeclaration since TypeScript knows that all possible cases have been covered so it thinks the code is unreachable. It can be reached forcefully as evidenced in one specific test case. I removed this since TypeScript will never allow you to use a false keyword for a variable identifier.

@hazzard993
Copy link
Copy Markdown
Contributor Author

You can now mix and nest array/object destructuring patterns.

let [a, { really, strange, tbl: { data, structure } }] =
    [0, { really: 1, strange: 2, tbl: { data: 3, structure: 4 }}];
print(a, really, strange, data, structure);     // 0, 1, 2, 3, 4

This can also happen inside functions/methods.

You can't use ellipse destructuring for objects or arrays.

let {...x} = {x: 1, y: 2};  // Disallowed
let [...x] = [0, 1, 2];     // Disallowed, this happened before

I've kept the behaviour of TupleReturn the same.

And instead of throwing an exception when there are nested bindings the transpiler falls back to this new method.

Copy link
Copy Markdown
Member

@Perryvw Perryvw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like these changes, also happy with the tests. I added a few minor suggestions regarding naming and style, but overall very good job!

// Binding patterns become _table0, _table1, etc as function parameters
// See transformFunctionBody for how these values are destructured
const paramName = ts.isObjectBindingPattern(param.name) || ts.isArrayBindingPattern(param.name)
? tstl.createIdentifier(`_table${identifierIndex++}`)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think _table is a good name, to be consistent with the rest of our extra variables I would prefer something like ____TS_bindingPattern

const bindingPatternDeclarations: tstl.Statement[] = [];
parameters.forEach(binding => {
if (ts.isObjectBindingPattern(binding.name) || ts.isArrayBindingPattern(binding.name)) {
const identifier = tstl.createIdentifier(`_table${identifierIndex++}`);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as other comment regarding name

// nested binding pattern
const propertyName = isObjectBindingPattern
? element.propertyName
: ts.createNumericLiteral(`${index}`);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this `${index}` just to cast to string, I think String(index) would be better.

const isObjectBindingPattern = ts.isObjectBindingPattern(pattern);
let index = 0;
for (const element of pattern.elements) {
index++;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this loop, might as well just make this:

for (var index = 0; index < pattern.elements.length; index++) {
    const element = pattern.elements[index];

Copy link
Copy Markdown
Collaborator

@tomblind tomblind left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really good stuff

@tomblind tomblind merged commit da0056a into TypeScriptToLua:master Feb 1, 2019
@hazzard993 hazzard993 deleted the objectbindingpatterns branch February 1, 2019 22:42
@hazzard993
Copy link
Copy Markdown
Contributor Author

Thanks for the reviews guys!

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 this pull request may close these issues.

3 participants