Duplicated nodes in object destructuring node about babylon HOT 13 CLOSED

This is intentional. What behaviour are you proposing?

from babylon.

The one which does not repeat Identifier twice and does not require changing it in two places when modifying AST I guess :)

from babylon.

That's not very helpful, what node structure are you proposing?

from babylon.

First thing comes to my mind.

ObjectPattern {
  properties: [ AssignmentPatternProperty | Property ]
}

PropertyAssignmentPattern <: Node {
  key: Identifier
  pattern: Pattern
}

from babylon.

To be even more precise, Property does not really match ObjectPattern well since it had a lot of fields that are not relevant to ObjectPattern. I predict ObjectPatternProperty in some bright future.

from babylon.

It's going to be a breaking change, while we do have that luxury since we forked the AST from ESTree it's still tricky because we have existing transforms and Babel rarely has a major.

The existing node definition is good for transformations as it doesn't require every single transform that targets object patterns to know how and when to perform explosion of shorthands. The idea of making shorthands distinct is obviously appealing to the idea of a CST but I'm unconvinced that it's an appropriate modification to an AST that's (currently) targeted so generally at transforms.

I understand it's annoying but IMO the pros far outweigh the cons as it leads to more reliable and solid transforms. I'm not convinced that the extra code needed to handle it is burdensome, for reference here is the code in babel-generator that handles it.

from babylon.

@markelog Can you elaborate on the complexity around traversal that you mentioned in the issue body?

from babylon.

You will get two and same Identifiers while traversing when it seems there should be only one and also see https://github.com/cst/cst/blob/996ce8451d1bdf14e06439533499d2e320cb3342/src/elementTree.js#L105-L119

from babylon.

And there is an example of code generation issue - cst/cst#127, since there is two identifiers they are duplicated

from babylon.

Those issues just seem to be consequent of the fact that it uses the AST in conjunction with tokens rather than relying entirely on the AST.

from babylon.

Those issues just seem to be consequent of the fact that it uses the AST in conjunction with tokens rather than relying entirely on the AST.

Yes, that's part of the issue, same kind thing could be viewed with any traversal though, like:

let i = 0;

let ast = require("babylon").parse("({ test = 1 } = {})", {});

let result = require('estraverse').traverse( ast.program, {
    keys: require("babel-types").VISITOR_KEYS,
    enter: function (node) {
        if (node.type === 'Identifier') {
            i++;
        }
    }
});

console.log(i); // 2

Or if you found a token and you want to understand for which node this token belongs to, you can find it by comparing position of the node and token since it has belong to the only one node right? That would be the assumption i guess, but in this case it wouldn't be true.

I understand you don't want to change that because you don't need this and we are only one consumer of this AST who is mentioned it, so there is could be any justification for denial really :).

from babylon.

so there is could be any justification for denial really :).

By that, i mean if you don't want to change then no big deal, we would be just force to add couple hacks, in any case it seems you made your mind already

from babylon.

Since it looks like we need an AST change that we'd not like to do with other changes in bulk, closing

from babylon.