Issues with `isolatedDeclarations` and the associated fixes in editor

[ssalbdivad]

🔎 Search Terms

isolatedDeclarations, quickfix, lag, editor, VSCode, 5.5

🕗 Version & Regression Information

• This is specific to new 5.5 features

⏯ Playground Link

No response

💻 Code

_

🙁 Actual behavior

I've been testing out isolatedDeclarations in VSCode Insiders on WSL and wanted to collect a few of the issues I noticed in general, especially with some of the associated quick fixes as discussed with @jakebailey in the TS Discord.

1. The most egregious issue I encountered was a huge lag spike when calculating quick fixes for isolatedDeclarations, even for simple types.

Here you can see I can hover this.to?.traverseApply which is trivially inferred as a simple type from the base class. But then after I hover outValidator and it has to generate a quick fix, it's like it gets stuck recalculating it:

https://github.com/microsoft/TypeScript/assets/10645823/88f3dbed-a99f-459e-92d7-fe47dea0f3fe

2. I also notice when I'm annotating a value with an isolated declarations error, sometimes auto import suggestions can take several seconds to pop up, so it seems like a general performance issue with some of these fixes.

More broadly, I also notice the total check time for ArkType's repo has increased from 7.5 seconds in 5.4 to 9.5 seconds in 5.5, so there may be some underlying change to e.g. calculate additional predicates, but these fixes associated with isolatedDelcarations seem to be particularly problematic.

Another pattern I noticed was lots of inline imports when applying a quick-fix which is rarely desirable. It would be great if by default (e.g. if there are no naming conflicts), the import would be added to the top of the file and the type would be referenced as normal:

https://github.com/microsoft/TypeScript/assets/10645823/6c5c1c43-25e9-421e-868d-7abad2109db6

3. I use the pattern type SomeType<parameter = SomeDefault> frequently, and I notice whenever a quickfix is applied it adds the parameter even if it's the default.

This wouldn't be a huge deal, but since if SomeDefault is a type, it will also be expanded out to its structural form and potentially inline-imported, it can add a ton of visual clutter (sometimes an order of magnitude more than the type itself).

4. I don't understand well most of the underlying rules but I think my biggest wish for isolatedDeclarations is more constants like those strings initialized as class props or stuff like this could somehow not require duplicating the entire object structure at a type-level to replicate what typeof someConstant could do in the past:

If that's fundamentally in conflict with the goals of --isolatedDeclarations, feel free to disregard, it just feels frustrating to have no way to derive the type and value from a single source without repeating them anymore even for simple cases.

Sorry for the haphazard format of this issue. I'm very excited about this feature as some of these kinks are ironed out I think the fixes will also add a lot of value, I just wanted to make sure there was a record of these things, particularly since Jake requested I submit something.

🙂 Expected behavior

_

Additional information about the issue

No response

[RyanCavanaugh]

Sorry for the haphazard format of this issue.

I actually do need this turned into separate concrete actionable items.

[DanielRosenwasser]

I've turned (3) into its own issue: https://github.com/microsoft/TypeScript/issues/58449

May or may not be something we can address easily.

[DanielRosenwasser]

For (4), that's a known pain-point. I'm not sure what the right solution there is. In that case, I would begrudgingly convert that to a module (or in reality I'd use an enum, but people have mixed feelings on that).

What would be nice is if we had something like

export const TIME_RATIOS: preserveinit = Object.freeze({
  ns: 0.000_0001,
  us: 0.001,
  ms: 1,
  s: 1000,
});

to say "actually preserve the entire expression in the declaration file". That means that anyone who runs this has to have Object.freeze globally in scope - which is obviously not a problem for most people in this case, but could be for certain other libraries. JSX also raises a weird question here (because now you possibly need .d.tsx).

[ssalbdivad]

@DanielRosenwasser Thank you very much and I'm sorry I'm a bit swamped at the moment or I would have done this initially. Apologies @RyanCavanaugh.

I have no intuitions about what is actionable here so if it's primarily #3 feel free to close this issue any time.

[DanielRosenwasser]

Nobody reacted with a 😕 or 👎 on my preserveinit idea- but I'm not even sure if I like it. I might convince myself to file an issue if someone hasn't already done it.

I think the first actionable thing is to figure out where we can defer some work on these quick fixes.