Skip to content
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

Issues with isolatedDeclarations and the associated fixes in editor #58426

Open
ssalbdivad opened this issue May 3, 2024 · 5 comments
Open
Labels
Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag

Comments

@ssalbdivad
Copy link

ssalbdivad commented May 3, 2024

πŸ”Ž 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:

Code_-_Insiders_BHWOQgSuwq.mp4
  1. 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:

Code_-_Insiders_gBTyHQRaop.mp4
  1. 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).

  1. 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:

image

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
Copy link
Member

Sorry for the haphazard format of this issue.

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

@DanielRosenwasser
Copy link
Member

I've turned (3) into its own issue: #58449

May or may not be something we can address easily.

@DanielRosenwasser
Copy link
Member

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
Copy link
Author

@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
Copy link
Member

DanielRosenwasser commented May 6, 2024

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.

@weswigham weswigham added the Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag label May 14, 2024
blickly added a commit to blickly/TypeScript that referenced this issue May 30, 2024
It turns out that `typeChecker.getEmitResolver()` can be expensive,
so avoid calling it unless necessary.

Hopefully this fixes the performance issue (bullet microsoft#1) of
microsoft#58426
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Domain: Isolated Declarations Related to the --isolatedDeclarations compiler flag
Projects
None yet
Development

No branches or pull requests

4 participants