-
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
Improving the AST for computed-name properties #266
Comments
Judging by how estree focuses on stability of the api, i think the
option is out of the question. If you want an ast format that takes that approach, then the Shift AST is something to look at, if you weren't currently aware of it. For example, for your example, its representation is the following: Shift AST JSON{
"type": "ObjectExpression",
"properties": [
{
"type": "DataProperty",
"name": {
"type": "StaticPropertyName",
"value": "prop"
},
"expression": {
"type": "LiteralNumericExpression",
"value": 2
}
},
{
"type": "DataProperty",
"name": {
"type": "ComputedPropertyName",
"expression": {
"type": "IdentifierExpression",
"name": "prop"
}
},
"expression": {
"type": "LiteralNumericExpression",
"value": 2
}
}
]
} As far as the first possibility goes, there's also Overall, i think the ideal solution for your use case (guessing that you're working in typescript) is to improve the typescript definitions you're using with something like type ObjectPropertyMaybeComputed = ObjectProperty & (
| { computed: false; key: Identifier | StringLiteral | NumericLiteral }
| { computed: true; key: Expression }
); |
Using another AST representation is entirely out of the question here, as I work on @typescript-eslint.
We've already done this, and have had types for this for some time. However it doesn't solve the problem. That's 100% true - |
Yeah, no worries, I hadn't noticed that you were working on typescript-eslint until after i mentioned the Shift AST. I think this is just a problem we have to live with in ESTree, as |
Unfortunately, we just can’t make changes like this to ESTree. As I’ve mentioned in other issues, ESTree isn’t really a spec that people follow, it’s more of a loose agreement between AST producers/consumers about how things should work. We can’t make breaking changes to ESTree because there’s no way to synchronize making such changes across every ESTree implementation. In the worst case tools stop working together and in the best case consumers end up needing to check for two different possible ways of doing the same thin, which will also introduce bugs. I personally hate the way that object literal members were defined in ESTree and wish we had something better, but by the time we started discussing it, there were already too many implementations out there to get an agreement to change it. So,in general, we need to live with the ugliness of existing parts of ESTree and just try to do better when we are able to add new nodes. |
One of the more difficult pieces of the AST to work with is properties. Why? Because computed-name properties and statically-named properties are a single AST node.
This is a huge footgun that most people don't think about when writing static analysis code or transforms on top of the AST. I see a lot of code which looks like this:
This code looks correct at a quick glance, until you realise that if
prop.computed === true
, then anIdentifier
has a very different meaning:Often times these bugs lie dormant in code as computed properties aren't super-regularly used (in my experience), but the bug can cause some really frustrating linting experiences - or in the case of code transforms it can lead to broken code in production.
There's obviously no truly backwards-compatible way of fixing this. I have two ideas for how the AST might be updated to fix this.
Create a new node for computed-name properties
Currently my best idea is to create a new node like:
The reason you'd probably want a new key entirely is to stop people just updating to this pattern and falling into exactly the same state that existed before:
PROs:
SpreadElement
s will continue to work correctly if the guard wasprop.type === 'Property'
.CONs:
SpreadElement
, is instead aProperty
" will crash because.key
is now potentiallyundefined
.Create a new node just for the property key
Another idea would be to rework the
.key
property:Again - the two types have different property names to prevent passthrough chaining like:
PROs:
prop.key.type === 'Identifier'
will now break.CONs:
I'd love to hear some thoughts about this.
Is this too much of a breaking change? Is this just an AST state that we need to grit our teeth and bear? Or is there something we could do about this?
The text was updated successfully, but these errors were encountered: