Add shorthand property to ExportSpecifier and ImportSpecifier?

[fisker]

import {foo} from 'foo'

Without location info, it's impossible to know if it's shorthand, if I understand correctly, SourceLocation is not required in ESTree.

Add shorthand property to ExportSpecifier and ImportSpecifier will make it easier to use.

[bradzacher]

What do you mean by "shorthand"? Do you mean {foo} vs {foo as bar}?
If so you can tell that by comparing the .imported and the .local properties on the ImportSpecifier node.

https://github.com/estree/estree/blob/master/es2015.md#importspecifier

If the names are exactly the same - does it matter at all? From an AST point of view it doesn't - {foo} and {foo as foo} are semantically the same.

IIRC you'll see the same behaviour when comparing import {} from 'mod' and import 'mod' - the ast is the same and it makes no semantic difference which one is written.

[fisker]

Sorry, I didn't make it clear, yes I mean {foo} vs {foo as foo}, yes they are the same, but when I work in Prettier project, and eslint rules, I need to know how it's written.

I just improved the shorthand specifier detection logic in Prettier, but I feel it will be much easier, if we have .shorthand like Property.

import {} from 'mod' and import 'mod' also causing problems in Prettier, and we haven't found a good solution yet... But that's not what I'm seeking in this proposal.

[RReverser]

Hmm. In general, this seems reasonable - we do have shorthand on Property, so why not here.

[nzakas]

Im not sure how much value a shorthand property adds at this point. Because existing ESTree producers may not update their implementation, you can’t just use if (node.shorthand) and would always need a fallback check to see if the local and imported nodes are the same.

[RReverser]

would always need a fallback check to see if the local and imported nodes are the same

It's not the same though? Such fallback doesn't convey information about whether the property was { foo: foo } or { foo }. Depending on the use-case, consumer code can always treat shorthand: undefined as either false or true when local === imported depending on what they want to do with that flag information.

[JLHwung]

IMO an AST should not differ semantically equivalent codes, such as import {} from 'mod' vs import from 'mod', and we have more such examples:

// single binding arrows
x => {}
(x) => {}

// trailing commas
[x]
[x,]

f(x)
f(x,)

// parenthesized expressions
// (directly within expression statement)
x
(x)

I guess they also introduced challenges to the prettier project. If prettier wants to have fine-grained info about the source input, I wonder if we can create a CST spec (like @RReverser mentioned in #219 (comment) before) extending from ESTree under a new project(ESTwig?), the CST spec can then handle whitespaces and comments as well.

ESTwig is ESTree + extra node types and properties. ESTwig must be compatible with ESTree, so a tool supporting ESTwig automatically supports ESTree.

[fisker]

@JLHwung I understand your concerns and they make sense. I don't think I would request this if there isn't shorthand in Property. Do you think we should remove that?

[nzakas]

@RReverser my point is that adding shorthand means you’ll always need to check shorthand and local vs imported because you’ll always need to take into account that shorthand can be undefined. So, adding shorthand helps in exactly one case: when an implementation has added it and it’s true.

When Acorn changed the shorthand object properties so key and value were different objects instead of the same one, we ended up needing to compare node locations to determine if they were the same. You can do the same thing with local vs exported in imports.

Adding shorthand makes sense logically for consistency with object properties, but I just don’t think it adds much from the perspective of ESTree consumers because it’s not safe to assume it will be produced from all ESTree parsers. There are plenty of places where we could add additional properties to node types to make life easier, but that’s a rabbit hole I don’t think is worth going down for all the same reasons I’ve already mentioned.

@fisker we can’t remove existing node properties without breaking a lot of existing ecosystem projects.