feat(FormFiledGroupHeaderTitleTextObject): interface renamed (typo) #610
Conversation
adamviktora
requested review from
wise-king-sullyman,
thatblindgeye and
Dominik-Petrik
| TSTypeReference(node: { typeName: Identifier }) { | ||
| if (!hasAlias(interfaceImport) && node.typeName.name === previousName) { | ||
| callContextReport(node as unknown as Node, node.typeName); | ||
| } | ||
| }, | ||
| TSInterfaceHeritage(node: { expression: Identifier }) { |
There was a problem hiding this comment.
Having these typed as objects with just the expected values and then asserting that they're Nodes feels odd, are there not better types we could use here?
There was a problem hiding this comment.
Yeah, I know. I was struggling to find any type for these, there are only enums in /packages/eslint-plugin-pf-codemods/lib/rules/ast-node-types.d.ts which I think is copied from @typescript-eslint/parser. But I am taking a look at the typescript-eslint repo now and it seems the actual interface might be exported there.
I'll try to import that
There was a problem hiding this comment.
So, the type we want is in this file: typescript-eslint repo, but it is a part of an internal package ast-spec.
After installing typescript-eslint, it does not export any of the AST node types from the node_modules.
Trying to install the internal ast-spec package directly with yarn add @typescript-eslint/ast-spec --dev does not work.
There was a problem hiding this comment.
Hm 🤔
Let me do a little hunting myself and see if I can find the types we need exported anywhere.
There was a problem hiding this comment.
I think I found them:
import { TSTypeReference, TSInterfaceHeritage} from '@typescript-eslint/parser'
There was a problem hiding this comment.
I tried importing this, the '@typescript-eslint/parser' package should be installed, but it says it is not exported.
Updated packages/eslint-plugin-pf-codemods/package.json:
Then I did yarn install
I don't know if I am installing something wrong, but it just seems like it is not exported.
There was a problem hiding this comment.
I'm seeing the same thing but I swear this was working on Friday 🤔
There was a problem hiding this comment.
I really am sure that this was working Friday and now I'm so confused 😆
It looks like types of those names can be retrieved from import { TSESTree } from '@typescript-eslint/types' ... but they don't slide right into place and I'm not sure if those are the wrong type or if the current logic isn't fully type safe.
I do feel like we should investigate this further but I won't block this PR over it if you'd like to make a followup issue so that we don't forget about it.
There was a problem hiding this comment.
Typescript contributors are playing some high-level games with us I guess 😃
Anyways, the TSESTree.AST_NODE_TYPES enum si fine, but it does not fit the use case as you say. I'd also not block over, the codemod should work even if typed as TSTypeReference(node: any) {, as if the node matches that TSTypeReference function, it should already have the suitable structure I suppose.
Opened the issue for further investigation: #622
There was a problem hiding this comment.
Yeah the codemod will still work, which is why I'm fine with the typing being patched in a followup. It just isn't ideal from a typing perspective since it isn't really valid, we're just telling TS to trust us and chill out 😆
There was a problem hiding this comment.
Looks good, opened #623 to possibly add logic to update an instance where the object is being exported as well.
Closes #609