-
Notifications
You must be signed in to change notification settings - Fork 358
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add shorthand
property to ExportSpecifier
and ImportSpecifier
?
#265
Comments
What do you mean by "shorthand"? Do you mean 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 - IIRC you'll see the same behaviour when comparing |
Sorry, I didn't make it clear, yes I mean I just improved the shorthand specifier detection logic in Prettier, but I feel it will be much easier, if we have
|
Hmm. In general, this seems reasonable - we do have |
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 |
It's not the same though? Such fallback doesn't convey information about whether the property was |
IMO an AST should not differ semantically equivalent codes, such as // 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. |
@JLHwung I understand your concerns and they make sense. I don't think I would request this if there isn't |
@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. |
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 toExportSpecifier
andImportSpecifier
will make it easier to use.The text was updated successfully, but these errors were encountered: