fix: inject Astro global types into TS programs#2983
Conversation
|
✅ Deploy Preview for flint-fyi ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
JoshuaKGoldberg
left a comment
JoshuaKGoldberg
left a comment
There was a problem hiding this comment.
LGTM! But I'm not particularly familiar with Astro globals / Volar, and would want a second opinion for sure.
| astro: { | ||
| ast, | ||
| // Astro's language server injects these same type files through getScriptFileNames: | ||
| // https://github.com/withastro/astro/blob/main/packages/language-tools/language-server/src/core/index.ts#L31-L74 |
There was a problem hiding this comment.
[Docs] To make this a permalink ~
| // https://github.com/withastro/astro/blob/main/packages/language-tools/language-server/src/core/index.ts#L31-L74 | |
| // https://github.com/withastro/astro/blob/c0f33eda8adf6f8f2588688f6205b76a96a42466/packages/language-tools/language-server/src/core/index.ts#L31-L74 |
JoshuaKGoldberg
added
the
1 approval
| // Astro's language server injects these same type files through getScriptFileNames: | ||
| // https://github.com/withastro/astro/blob/main/packages/language-tools/language-server/src/core/index.ts#L31-L74 | ||
| function addAstroTypes( | ||
| ts: typeof import("typescript"), |
There was a problem hiding this comment.
Recommend naming this something else or using named imports at the top of the file (my preference). ts is already declared in the module scope, and having it named the same here can be confusing.
There was a problem hiding this comment.
Auvred and I have bikeshedden this a lot, we don't really see a better option because of TS's shadowing rules. But, @michaelfaith, please give it a test and if you can find a version that works, file an issue/PR to make them consistent.
Sorry if that feels short, long day.
There was a problem hiding this comment.
I'm not sure I understand that. Wouldn't a better option be what I suggested: using named imports at the top instead of a default import named ts? Or the other suggestion I said, which is changing the name of this parameter to something else (e.g. typescript) so it doesn't "shadow" the default import ts?
There was a problem hiding this comment.
I see what you're saying 👍🏻
I was thinking about replacing the typeof import("typescript") but that works as well.
There was a problem hiding this comment.
I think it's fine to leave for now though, this is how all of the adapters are shaped. If you want it changed, might as well do it everywhere.
4 participants
PR Checklist
status: accepting prs: N/A — no issue.Overview
Adds Astro's global type declarations to TypeScript programs that contain
.astroroots, matching Astro language-server's program-level type injection instead of patching generated virtual TSX.Verification run locally:
pnpm --filter=@flint.fyi/astro-language test,pnpm build,pnpm lint,pnpm flint,pnpm lint:knip,pnpm lint:knip:prod, andpnpm dedupe --check --prefer-offline.