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

fix: allow JS snapshots within TS plugin #2565

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dummdidumm
Copy link
Member

So far we've always assumed that a Svelte file is a TS file for simplicity, due to our default language (which was removed a long time ago) and also because IIRC there were issues with TS having its snapshot scriptKind being switched. That seems to be no longer the case, and so we can get better at properly analyzing whether or not this is a TS file, to allow JSDoc to take action when it's a JS file.

#2555

@jasonlyu123 do you remember why that scriptKind switch broke previously, but apparently doesn't anymore? I vaguely remember us having the same problem within the language server, but you PRd a change at some point stating that it's no longer a problem.

Also, is this a breaking change, because if someone has no allowJS in their config they would get errors?

So far we've always assumed that a Svelte file is a TS file for simplicity, due to our default language (which was removed a long time ago) and also because IIRC there were issues with TS having its snapshot scriptKind being switched. That seems to be no longer the case, and so we can get better at properly analyzing whether or not this is a TS file, to allow JSDoc to take action when it's a JS file.

#2555
@dummdidumm dummdidumm marked this pull request as draft November 5, 2024 17:15
@jasonlyu123
Copy link
Member

jasonlyu123 commented Nov 6, 2024

Yes, there is a Debug.assert for ScriptKind changes. TypeScript added a fix for it some time ago. The Debug.assert is still there, so it might still happen in some edge cases, but it shouldn't happen in most cases. #2538 is an example. In ts-plugin, you'll need file save to trigger updates, so for #2538 to happen in ts-plugin, it is edge case + edge case. Also, The more reliable reproduction for #2538 should only happen in module resolution "node16"+ because of the impliedNodeFormat patch but we didn't patch it in ts-plugin.

As for allowJs, I am leaning toward this as a correctness fix. It also should only affect files loaded through import and not in project files because we can't know if the files are lang="ts" without reading it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants