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

"Initializing JS/TS languages features" never completes after upgrading from 6.4.0 to 6.5.1 #415

Closed
OliverJAsh opened this issue Oct 5, 2021 · 11 comments
Labels
🐛 Bug Refactoring breaks existing behavior

Comments

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Oct 5, 2021

Describe the bug

After opening a TS file, "Initializing JS/TS languages features" never completes after upgrading from 6.4.0 to 6.5.1.

How to reproduce

I tried to reduce this down to a small test case but I didn't have much luck. I decided to file an issue to see if other people are also running into this and also to ask for any debugging tips.

I was able to reproduce it in a fresh instance of VS Code where Abracadabra was the only extension installed.

I tested this in a large repo, however I was not able to reproduce the issue in a much smaller repo.

Expected behavior

"Initializing JS/TS languages features" completes

Screenshots

image

Additional information

  • Version of the extension impacted: v6.5.1
@OliverJAsh OliverJAsh added the 🐛 Bug Refactoring breaks existing behavior label Oct 5, 2021
@OliverJAsh
Copy link
Contributor Author

If I open developer tools in VS Code, I see this in the console:

image

I suspect this has something to do with the new type-checker module added in #414.

As a side note, it looks like the extension is creating another instance of the TS language service. This is not recommended as it is bad for performance. Instead, the extension should hook into the existing TS language service that VS Code is already running, via a language service plugin.

@nicoespeon
Copy link
Owner

Thanks for reporting @OliverJAsh 👍

I tested this in a large repo, however I was not able to reproduce the issue in a much smaller repo.

Ah, that's why I wasn't able to repro myself. I will try it out on a large repo to see the impact.

It's very certainly related to the new TypeChecker logic we implemented. It's the major "new" element of the codebase. It probably has to do with the instantiation of a new TS service indeed. The end goal is to tap into the existing one that VS Code uses.

But I thought I could create one on the fly since the scope is limited to a single file: we are using a virtual instance with only the code of the file being processed

private createTSProgram(): ts.Program | undefined {
const host: ts.CompilerHost = {
...ts.createCompilerHost({}),
getSourceFile: (fileName, languageVersion) =>
ts.createSourceFile(fileName, this.code, languageVersion)
};
return ts.createProgram([this.fileName], {}, host);
}

The impact on performance is probably worse than I thought. It may be related to the combination of this and the Action Provider. If so, a quick fix could be to remove the Action Provider until we use VS Code API.

I will try that out on a big repository, run some performance analysis, and find the root cause.

@nicoespeon
Copy link
Owner

In the meantime, I'm gonna remove the Action Provider. It's a guess but it may solve the problem for you until I sort this out.

You will still be able to run "Destructure Object" through the Command Palette, but it won't appear in the lightbulb suggestions anymore for now.

@rsxdalv
Copy link

rsxdalv commented Oct 5, 2021

For those looking for a quick fix, here's a reminder of how to install previous 6.4.0 version:
image

@OliverJAsh
Copy link
Contributor Author

OliverJAsh commented Oct 5, 2021

Thanks for the quick reply! ❤️

But I thought I could create one on the fly since the scope is limited to a single file: we are using a virtual instance with only the code of the file being processed

Even if you're only checking the types for one file at a time, I imagine TS would still need to resolve the dependency graph of that file in order to compute the type of a given node.

@nicoespeon
Copy link
Owner

Oh my, what have I done 🤦‍♂️

CleanShot 2021-10-05 at 08 20 16

That is taking an insane amount of time, repeating in a loop.
Alright, disabling the Action Provider for this refactor does fix the performance issue while giving you the option to trigger the refactoring when you need it.

I'm shipping a new patch version with this fix so you can use your editor again. Sorry for the trouble.

I'll fix the perf issue, it's quite obvious what is going wrong here.

@nicoespeon
Copy link
Owner

@OliverJAsh yeah, the perfs of creating a new TS instance isn't great. It's usable and fine on small files, but takes hundreds of ms (at least) on bigger ones.

The major issue is that I exposed it as a quick fix (Action Provider). VS Code runs that logic every time you move around the file. So expensive calculation + constantly running it = 💣

I didn't catch it before because I was working with relatively small files on a performant machine, so I didn't notice.
Shipping a patch now 🚀

@nicoespeon
Copy link
Owner

nicoespeon commented Oct 5, 2021

New release created: https://github.com/nicoespeon/abracadabra/releases/tag/6.5.2

It should be available in a couple of minutes 👍

I'll create a specific issue title to track the perf fix of this refactoring, so this one can be retrieved by people who would be stuck in the 6.5.1 situation.

Well, I wanted to dig into accessing VS Code's TS instance anyway. That would unlock multi-files refactorings. I guess now is a good time to dive in 🔦

@nicoespeon
Copy link
Owner

@OliverJAsh can you let me know if 6.5.2 solves the issue?

@OliverJAsh
Copy link
Contributor Author

It does, thank you! ❤️

If you do look into language service plugins, I would be very interested to follow along 😄

@nicoespeon
Copy link
Owner

🙌 Good to hear! Closing the issue then.

I'll definitely look into Language Service plugins. This seems to be the best way to achieve it:

I'll let you know about it 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 Bug Refactoring breaks existing behavior
Projects
None yet
Development

No branches or pull requests

3 participants