-
-
Notifications
You must be signed in to change notification settings - Fork 192
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
(fix) prevent ts plugin crash when config updated #2019
(fix) prevent ts plugin crash when config updated #2019
Conversation
if (p === sveltePluginPatchSymbol) { | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the if (!configManager.getConfig().enable || p === 'dispose') {
below still correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I don't remember why dispose
has special treatment here. But it seems wrong. Maybe at some point in the original PR dispose
is patched before proxy is created, or something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember why dispose
has special treatment here now. It should run the decorated version even if the typescript-plugin is disabled, but the current check is wrong.
another language service object which still has our patches applied? Because if not, what would happen how? |
Yes. But not the same object. Debugging show that they use |
Do they just copy over everything? In other words, if we didn't use a symbol but a |
Yes. But TypeScript will copy over that property no matter if the plugin has extended our patch (proxy Inception lol). Once we add that property, it'll be there until the language service is destroyed. So it's not much better than checking patched project names. But it's a more direct check. I can change to that if you prefer that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's moved over due to the key thing anyway, then this change is probably for the better 👍
@Princesseuh FYI, Astro might also have this problem. |
#2018 sveltejs/kit#9932
This happens when tsconfig or extending tsconfig updates and another ts-plugin returns another language service object. For example, https://marketplace.visualstudio.com/items?itemName=VisualStudioExptTeam.vscodeintellicode. Because the patched symbol is no longer there, patching will run again. And when we call
project.addRoot
, a Debug assertion error will be thrown as the file is already a root file. TypeScript will then removes the external files because the plugin failed to initialise.