pngwn / MDsveX

A markdown preprocessor for Svelte.
https://mdsvex.pngwn.io
MIT License
2.27k stars 96 forks source link

Add type definitions for *.svx files by default #286

Open devmattrick opened 2 years ago

devmattrick commented 2 years ago

Currently, MDsveX doesn't provide a way for TypeScript to really understand .svx files. The way Svelte deals with this issue is by declaring a global ambient module (in their ambient.d.ts file. Since `.svx` files from what I can tell are just disguised Svelte components, this is what I'm using currently to workaround the IDE errors I'm getting:

global.d.ts

// Based on svelte/runtime/ambient.d.ts
declare module "*.svx" {
    export { SvelteComponentDev as default } from "svelte/internal";
}

But it'd be awesome if this could be included in the mdsvex package or something to not require this sort of thing. Thanks!

pngwn commented 2 years ago

I'd actually like proper typescript support for svx files. This will require language server integration. Ambient declaration will be good solution until I can figure out the details of better support.

vhscom commented 2 years ago

Given the following module declaration provided by the OP:

// global.d.ts
declare module "*.svx" {
  export { SvelteComponentDev as default } from "svelte/internal";
}

The following needs to be added to address the metadata named export:

declare module "*.svx" {
  export { SvelteComponentDev as default } from "svelte/internal";
+ export { metadata };
}

I suspect the first export is either (a) not needed or (b) can be simplified but without knowing which errors OP was hitting in their use case it's hard to know until writing enough code against this lovely MDX-for-Svelte library to see which errors might jump out.

PuruVJ commented 2 years ago

I'd like to take a stab at it

Because these are ambient types, I think we'd need to have another globals.d.ts file in the exports, and give instructions to add <reference types="mdsvex/globals" or add it to types field of tsconfig.json.

What do you think?

(On an off-note, I'd like to add a defineMDSvexConfig helper function to make the config more type safe when used with svelte-add(it makes a mdsvex.config.js, which I highly prefer over putting it in svelte.config.js), we can discuss that after the ambient issue is fixed)

pngwn commented 2 years ago

Your proposal makes sense to me. Would happily accept a PR, I haven't looked into this at all.

Also an issue for config typedefs: https://github.com/pngwn/MDsveX/issues/300

PuruVJ commented 2 years ago

Got it! Will make separate PRs for both later today

pngwn commented 2 years ago

Hero.

PuruVJ commented 2 years ago

Hi, I have added the proper typings, how do I test it? Is there a demo folder or something?

PuruVJ commented 2 years ago

It's done! This issue can be closed

vhscom commented 2 years ago

Thanks for your effort, PuruVJ. But shouldn't we test it first? :smiley: I pulled down 0.10.2 and added the following pragma to my global.d.ts under the one included by SvelteKit:

  /// <reference types="@sveltejs/kit" />
+ /// <reference types="mdsvex/globals" />

Then removed my custom typings to handle .svx files and opened up a .svelte module importing a .svx file and I still see a type error. To reproduce pull down the void-app repo, make the changes mentioned above and then open src/layouts/AboutPage.svelte to see the type error return. Did I miss something?

PuruVJ commented 2 years ago

Strange, it's working fine for me. Reload your vscode once?

Does Ctrl+Click on the mdsvex/globals string take you to the d.ts file? If not, you may be a version behind

Edit: From the pnpm lock on your repo, mdsvex is still showing 0.9.8 everywhere, unless you haven't uploaded new changes?

CleanShot 2022-01-31 at 17 48 19@2x
PuruVJ commented 2 years ago

I did the same

CleanShot 2022-01-31 at 17 44 27@2x CleanShot 2022-01-31 at 17 44 23@2x
vhscom commented 2 years ago

Strange, it's working fine for me. Reload your vscode once?

That was the issue. I had only restarted the window and not the app itself (I'm using VSCodium). Thanks!

PuruVJ commented 2 years ago

Ah. Great it's resolved for you

pngwn commented 2 years ago

Resolved in #396 and #399.

pngwn commented 2 years ago

This has been reverted, last release broke the lib.

pngwn commented 2 years ago

Resolved in #409 released in 0.10.5