Mixed shorthand function with arrow function in object literal is mis-formatted

[pgilad]

Mixed object literal with shorthand functions and arrow functions get mis-aligned:
in:

var obj = {
    func1() {},
    func2: () => {}
};

out:

var obj = {
    func1() {},
        func2: () => {}
};

expected:

var obj = {
    func1() {},
    func2: () => {}
};

[bitwiseman]

Is this ES6? Please provide a link and exmples.

[pgilad]

Yeah this is ES6, sorry for not specifying that.
This is a link to a description of arrow functions:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions/Arrow_functions

and this example for usage:

var a = [
  "Hydrogen",
  "Helium",
  "Lithium",
  "Beryl­lium"
];

var a2 = a.map(function(s){ return s.length });
var a3 = a.map( s => s.length );

And this is a link to shorthand object literal:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Object_initializer
and example:

// Shorthand method names (ES6)
var o = {
  property([parameters]) {},
  get property() {},
  set property(value) {},
  * generator() {}
};

[bitwiseman]

Thanks for the report and clear information.
I've put this in the "maybe someday" bucket.
You are welcome to do a pull request for this, but we have much more glaring issues to fix in the short term.

[mansona]

I'm pretty certain this isn't just an issue with the Enhanced Object Literals for functions and not the arrow functions. We are having a similar issue but we are not using arrow functions.

e.g. in:

var obj = {
    func1() {},
    func2: function() {}
};

out:

var obj = {
    func1() {},
        func2: function() {}
};

These are just hand coded examples, I would love to add a test but I'm finding it hard to figure out where the test should go 😕

would you recommend adding it to this file: https://github.com/beautify-web/js-beautify/blob/master/js/test/beautify-javascript-tests.js

[jwhitmarsh]

I'd be great to see this fixed!

@bitwiseman I'd be interested in taking a look at the fix for this - could you point me in the right direction? (apart from going to /js/lib/beautify.js)

[r-murphy]

@bitwiseman I have a couple potential options to fix this, but I'd like some feedback.
Both options are on my fork at https://github.com/r-murphy/js-beautify.

Both options address the fact that mode ObjectLiteral is not detected when the first property is a shorthand function, so it remains as mode BlockStatement. I noticed in handle_start_block there is some logic to look ahead a couple tokens to determine if it is an ObjectLiteral. But I didn't attempt that approach here, since it would be looking ahead an unknown number of tokens if the function takes arguments.

The two options I did are:

1. In handle_comma, I expanded the existing parent.mode check to also see if the parent is a BlockStatement. Not ideal, since there shouldn't really be a comma after a BlockStatement close, but all tests pass.
2. Detecting ObjectLiteral. But rather than detecting it in the surrounding ObjectLiteral's handle_start_block, I detect it in the shorthand function's handle_start_block, and then set the mode for the surrounding block (flag.parent.parent.mode). I'm probably not taking the best approach to setting the parent mode here.

Thoughts?

[bitwiseman]

@r-murphy:
Thanks for taking a swing at this.
I commented on r-murphy@06e84ba.

IGNORE THE REST OF THIS COMMENT! Neither option 1 or option 2 are sufficient. See next comment.

Option 2 seems brittle.

Because the beautifier is not precise about parsing, it's okay to take reasonable guesses.
I would go with something like option 1, but rather than expanding the if like that, I'd do something like (starting at line 1085):

print_token();
// Some object literal cases are hard to detect.  
// Best to assume that when we see a comma in a block statement, it is actually an object literal. 
if (flags.mode === MODE.Statement && flags.parent.mode === MODE.BlockStatement) {
    flags.parent.mode = MODE.ObjectLiteral
}

if (flags.mode === MODE.ObjectLiteral ||
    (flags.mode === MODE.Statement && flags.parent.mode === MODE.ObjectLiteral)) {
    if (flags.mode === MODE.Statement) {
...

This will make other formatting that depends on ObjectLiteral work correctly. (This is what we used to do. I thought we could get away from it, but no.) For example, another test you should add:

var obj = {
    func1() {},
    a: 1,
    func2() {}
};

So, yeah, a modified option 1 and a few more tests. Please continue and submit a PR!

[bitwiseman]

Thinking about it for a bit more, do not use either of the options you've mentioned.

The most highly requested feature (#315) will require that we know if we're dealing with an ObjectLiteral at the start of that first curly-brace, not at some later point. That is why I have all the ObjectLiteral detection to occur at the start of the block.

So, you'll need to look for ways to detect ObjectLiteral at the starting curly. #812 is the most recent improvement, but I'm not sure how you're going to reliably detect this case. You could mostly close this by look at the surrounding context. In assignment or locations that expect expressions you get an Object, but after if, while, do, function (and so on) you get Blocks.

[r-murphy]

Thanks for the feedback. Agreed that detecting it in the handle_start_block is cleaner.

That's a good idea to use the surrounding context. Although certainly possible to detect the shorthand function by checking if the 2nd token is '(' and then looping forward, the loop exit conditions would have to be robust to avoid infinite loops in case of invalid javascript on the user's part. Checking the surrounding context is much safer.

I'm thinking to check the following whitelist of last_types, rather than trying to come up with all the block preceding keywords such as if, while, do, function etc...

} else if (in_array(last_type, ['TK_EQUALS', 'TK_START_EXPR', 'TK_COMMA'])) {
    // If the block is being assigned or passed to a function, treat as an ObjectLiteral
    set_mode(MODE.ObjectLiteral);
}

I currently have this after the (second_token && ...) condition, although they're not mutually exclusive.

Thanks

[bitwiseman]

That is a fine start but you're going to have to check for keywords. The following is way too common:

function someFunc() {
   return { ... };
}

The only way to determine that is an ObjectLiteral is to check if (last_type === 'TK_RESERVED' && last_text === 'return'). 😢
And you're going to need a test for each of the alternatives.

You might add a check for last_text === ':' as well.

I understand if this more than you wanted to take on. If you get tired of running down all the corner case, I'd be happy to see a partial fix go in as long is it has tests for the supported cases and includes the matching the python implementation.

Thanks again!

[r-murphy]

I can't believe I forget return { ... }; 😆
Our comments crossed paths for checking last_text === ':', so that one is handled.

I'll try to come up with more corner cases tomorrow and send a PR.

Thanks for the guidance and for the great tool.