ipatalas / vscode-postfix-ts

Postfix notation for TypeScript/Javascript - extension for VS Code
MIT License
159 stars 43 forks source link

Incorrect type snippet inserting with indexed access #62

Open zardoy opened 2 years ago

zardoy commented 2 years ago

I have .t snippet that I use to wrap types into generics e.g. SomeType.t -> Partial<SomeType>, its really handy. However, I usually use it in combination with Extract so I can reduce amount of fields passing to the generic above e.g. Partial.

Snippet:

{
    "name": "t",
    "description": "",
    "body": "$1<{{expr}}>$2",
    "when": ["type"]
}

Works:

Extract<Config.t, ''> -> Extract<<Config>, ''>

Fails:

Extract<Config[''].t, ''> -> <Extract<Config['']>, ''> // notice incorrect insertion of opening <

The same applies for typeof:

Partial<Extract<typeof config.t, ''>> -> Partial<<Extract<typeof config>, ''>>

Expected:

Partial<Extract<typeof config.t, ''>> -> Partial<Extract<<typeof config>, ''>>

Hope I made it clear.

zardoy commented 2 years ago

The last two examples also "fails" without Partial, just added it to demonstrate where exactly it inserts <. I find this behavior is inconsistent.

ipatalas commented 2 years ago

I admit I haven't been using generics too much so officially it's not supported. If it works then by coincidence :)

First two however seem to work as expected: generics

This might be tricky to get full support but I'll have a look on the AST of generics.

zardoy commented 2 years ago

Hey, @ipatalas regarding type snippets, is this by design that identifier postfixes are suggested in type references?

For example:

only type suggestions: type a = null type & identifier suggestions: type a = a

in ast it's still a valid Identifier, but as I understand they should be suggested for variableNames only

ipatalas commented 2 years ago

Well, the identifier condition is basically a map to TS identifier in AST and Type is also an identifier: image

I can see that README does not specify that very clearly and your interpretation makes absolute sense. The change should be relatively simple but it may be a breaking change for some users so I need to think if it's not gonna be a big deal.