feat(ast): heritage array is now optional, not empty array #120
Conversation
Codecov Report
@@ Coverage Diff @@
## master #120 +/- ##
=========================================
- Coverage 93.22% 93.2% -0.02%
=========================================
Files 8 8
Lines 1357 1368 +11
Branches 319 324 +5
=========================================
+ Hits 1265 1275 +10
Misses 51 51
- Partials 41 42 +1
Continue to review full report at Codecov.
|
BREAKING CHANGE: This changes the AST of TSInterfaceDeclaration nodes
|
@armano2 in e40b9d7 I have moved the ignore for abstract classes to node patching. My gut reaction is that aligning with babel here might not actually a step in the right direction. I feel like this is similar to other things we've had before where it makes the most sense for the construct to have its own unique node type... What do you think? I could imagine this would be harder to deal with in a linting situation, potentially causing issues with existing rules... |
|
i was thinking about this to, if we unify those nodes, there will be no easy way to query them in eslint. i will just suggest renaming them, ClassImplements to TSClassImplements (this is no es node), i will follow name of property from babel:
instead of heritage you can use something like this: TSExpressionWithTypeArguments(node: any, parent: any) {
if (parent.type === 'TSInterfaceDeclaration') {
node.type = 'TSInterfaceHeritage';
} else if (
parent.type === 'ClassExpression' ||
parent.type === 'ClassDeclaration'
) {
node.type = 'ClassImplements';
}
}and we should log suggestion/request on babel repo for this |
|
One thing is that TSExpressionWithTypeArguments has To be honest though, I'm struggling to create valid TypeScript where it is anything other than an |
|
@armano2 I have made all the changes we discussed, please could you take a look? |
src/convert.ts
Outdated
| }); | ||
|
|
||
| if (interfaceHeritageClauses.length > 0) { | ||
| result.extends = interfaceHeritageClauses[0].types.map(el => |
There was a problem hiding this comment.
i have only one concern here
interface foo implements bar extends foo {}
its going to pick up wrong heritage
There was a problem hiding this comment.
i know that whas a case before to, but prettier is going to mess up here
There was a problem hiding this comment.
Yep good point, I'll fix it
There was a problem hiding this comment.
there is also more broken example
interface foo extends bar extends baz {}
TS1172
| @@ -34,6 +34,8 @@ export interface ESTreeNode { | |||
| value?: string; | |||
| expression?: ESTreeNode | null; | |||
| decorators?: (ESTreeNode | null)[]; | |||
| implements?: ESTreeNode[]; | |||
| extends?: ESTreeNode[]; | |||
There was a problem hiding this comment.
extends can be array of nodes or node
extends in class is node, in interface is array
There was a problem hiding this comment.
It's superClass in classes right? Extends isn't used?
There was a problem hiding this comment.
ahh you are right, there was change with that :>
|
typescript also parses code like this correctly: interface foo extends foo.call {} // validinterface foo extends foo.call() {} // TS2499interface foo extends foo() {} // TS2499interface foo extends ({ foo: bar }) {} // TS2499 |
i'm starting think that expression is better suited for this because it can contain |
|
Pushing shortly, thanks for the updates, and good spot on the TSQualifiedName |
|
@armano2 let me know what you think of the last two commits |
|
Thanks! Let's stabilize for a bit now while we catch the ESLint ecosystem up and take stock |
|
all of nodes are now aligned -> most of issues is due to range bugs in babel :) i'm running my tests now |
|
🎉 This PR is included in version 18.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Well apart from all the property and kind tweaks we have to make to nodes in ast-alignment/utils right? :) |
|
yes, in my tests i'm doing same thing. |
I'll probably add the other heritage related alignment points before merging