unplugin / unplugin-icons

🤹 Access thousands of icons as components on-demand universally.
https://www.npmjs.com/package/unplugin-icons
MIT License
4.13k stars 144 forks source link

feat!: add Svelte5 specific types #381

Closed mattdavis90 closed 3 weeks ago

mattdavis90 commented 1 month ago

Description

When using the library with Svelte5 I noticed that I was getting errors when importing the icon components. This is because Svelte5 changed the type of components so I couldn't assign the icon components to a Component typed variable in typescript.

This PR updates the type exports to be in line with Svelte5 and removed all issues from my LSP and eslint.

Linked Issues

None that I could find

Additional context

None - happy to discuss further - I didn't open an issue first because it is a pretty trivial change and no real time lost if it isn't accepted. Thanks

stackblitz[bot] commented 1 month ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

userquin commented 1 month ago

This PR should be breaking, we need to redirect types/svelte.d.ts to types/svelte5.d.ts adding types/svelte5.d.ts to the package.json exports entry, just below types/svelte4.d.ts.

We also need:

We don't use that formatting, enable eslint or try running pnpm lint:fix:

svelte5.d.ts ```ts declare module 'virtual:icons/*' { import { Component } from 'svelte' import type { SvelteHTMLElements } from 'svelte/elements' export default Component } declare module '~icons/*' { import { Component } from 'svelte' import type { SvelteHTMLElements } from 'svelte/elements' export default Component } ```
mattdavis90 commented 1 month ago

At the point of filing the PR Svelte5 was still in RC so I didn't update the default but happy to make that change now that it is released. Ref linting, sorry thought I'd run eslint but apparently not.

userquin commented 1 month ago

Yeah, I just merged Svelte 5 support a few minutes ago in main branch (I mean the PR about adding svelte 5 in supported versions)

mattdavis90 commented 1 month ago

I force pushed so that my comment is in line with conventional commit messages. The examples look to be working correctly so remain unchanged

userquin commented 1 month ago

Add svelte5.d.ts to packages exports below this https://github.com/unplugin/unplugin-icons/blob/main/package.json#L112-L114

userquin commented 1 month ago

The example will require to update Svelte and some other dependencies (SvelteKit for example).

We also need to update examples/vite-svelte dependencies.

userquin commented 1 month ago

examples/sveltekit:

                "@sveltejs/kit": "^2.7.2",
        "@sveltejs/vite-plugin-svelte": "^4.0.0",
        "svelte": "^5.0.3",
        "svelte-check": "^4.0.5",
socket-security[bot] commented 1 month ago

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@sveltejs/kit@2.7.2 environment, eval +4 807 kB svelte-admin
npm/@sveltejs/vite-plugin-svelte@4.0.0 None +2 182 kB svelte-admin

🚮 Removed packages: npm/@sveltejs/kit@2.5.18, npm/@sveltejs/vite-plugin-svelte@3.1.1

View full report↗︎

mattdavis90 commented 1 month ago

I think I've got all the dependencies and updated to svelte5 runes in the example. Having a little trouble testing because pnpm isn't playing nice with the example workspaces

userquin commented 1 month ago

Yeah, in fact it is using Svelte 4, if you want to check it, just add a console.log below the version import with it and runnes flag:

console.log('Svelte version:', VERSION, 'svelteRunes:', svelteRunes)

image

userquin commented 1 month ago

pnpm lock file is missing in the PR and should be changed, doing weird things...

Maybe we should add empty pnpm-workspace.yaml to both svelte examples and exclude them from the workspaces (pnpm) like we did with nuxt3.

mattdavis90 commented 1 month ago

I got the workspace examples working, as you noted it was the missing pnpm-lock - no idea what my pnpm install was doing!

mattdavis90 commented 1 month ago

Both examples seem to be working now. I had a bit of a mess with my pnpm install. I cleared out all of the node_modules and they're both working now.

userquin commented 1 month ago

I got the workspace examples working, as you noted it was the missing pnpm-lock - no idea what my pnpm install was doing!

pnpm doing the same on my local (lock file unchanged)

userquin commented 1 month ago

/cc @dominikg @benmccann can you review this :pray: when you have some free time?

userquin commented 1 month ago

@mattdavis90 can you check svelte compiler? maybe we can also remove the ts-expect-error (eslint should be complaining about that annotation)

mattdavis90 commented 1 month ago

I'm still getting a TS error if I remove that annotation. I haven't added svelte to dependencies, peer, or dev - it is in the lock file though for the examples

userquin commented 1 month ago

We need to review svelte 5 dts, seems to be not working, there is no auto completion in vscode, svg attrs should be there.

mattdavis90 commented 1 month ago

I can't find a better type for it. Component is the new type for components and expects props as the first generic which SvelteHTMLElements['svg'] provides. Having said that you're totally correct and I also don't see any auto complete in neovim. I hadn't actually checked that - I mostly just wanted to remove the errors around Component vs SvelteComponent. Having tested for the last hour or so I can't get completion working

userquin commented 1 month ago

I can't find a better type for it. Component is the new type for components and expects props as the first generic which SvelteHTMLElements['svg'] provides. Having said that you're totally correct and I also don't see any auto complete in neovim. I hadn't actually checked that - I mostly just wanted to remove the errors around Component vs SvelteComponent. Having tested for the last hour or so I can't get completion working

imagen

mattdavis90 commented 1 month ago

Its now working for you? Did you change something? I still get no completion in nvim

userquin commented 1 month ago

declare module 'virtual:icons/*' {
  import { Component } from 'svelte'
  import type { SvelteHTMLElements } from 'svelte/elements'

  type Type<K> = {
    [P in keyof K]: K[P]
  }

  const component: Component<Type<SvelteHTMLElements['svg']>>

  export default component
}

declare module '~icons/*' {
  import { Component } from 'svelte'
  import type { SvelteHTMLElements } from 'svelte/elements'

  type Type<K> = {
    [P in keyof K]: K[P]
  }

  const component: Component<Type<SvelteHTMLElements['svg']>>

  export default component
}
userquin commented 1 month ago

Remove the type, just split the component + default export:

declare module 'virtual:icons/*' {
  import { Component } from 'svelte'
  import type { SvelteHTMLElements } from 'svelte/elements'

  const component: Component<SvelteHTMLElements['svg']>

  export default component
}

declare module '~icons/*' {
  import { Component } from 'svelte'
  import type { SvelteHTMLElements } from 'svelte/elements'

  const component: Component<SvelteHTMLElements['svg']>

  export default component
}

imagen

mattdavis90 commented 1 month ago

Cool - you totally beat me to it - this is what I was experimenting with. I hadn't got the keyof bit quite working yet

declare module "virtual:icons/*" {
  import type { Component } from "svelte";
  import type { SVGAttributes } from "svelte/elements";

  const c: Component<SVGAttributes<SVGSVGElement>>;
  export = c;
}
userquin commented 1 month ago

Cool - you totally beat me to it - this is what I was experimenting with. I hadn't got the keyof bit quite working yet

declare module "virtual:icons/*" {
  import type { Component } from "svelte";
  import type { SVGAttributes } from "svelte/elements";

  const c: Component<SVGAttributes<SVGSVGElement>>;
  export = c;
}

This is TS: export { c } should also work => import { c as XXX } from 'virtual:icons/...

mattdavis90 commented 1 month ago

Neither of your solutions give me completions in nvim but if they're working in VSCode them I'm happy to put it down to a client issue

userquin commented 1 month ago

Change svelte5.d.ts: next time we should check any d.ts at root ;)

userquin commented 1 month ago

Neither of your solutions give me completions in nvim but if they're working in VSCode them I'm happy to put it down to a client issue

Did you restarted ts service?

mattdavis90 commented 1 month ago

Neither of your solutions give me completions in nvim but if they're working in VSCode them I'm happy to put it down to a client issue

Did you restarted ts service?

Yeh, it works if I do Component<{abc: number}> but no suggestions for Component<SVGAttributes<SVGSVGElement>> or Component<SvelteHTMLElements['svg']> - I'll check the logs and restart nvim

mattdavis90 commented 1 month ago

It is working in a new project - looks like it is just broken in my local where I was overriding the types in app.d.ts

userquin commented 1 month ago

WebStorm/IntelliJ will require some update to support kit 5 :sweat_smile:

imagen

mattdavis90 commented 1 month ago

Thanks for the help :)

mattdavis90 commented 1 month ago

@mattdavis90 can you check svelte compiler? maybe we can also remove the ts-expect-error (eslint should be complaining about that annotation)

I just noticed in https://github.com/unplugin/unplugin-icons/pull/382 that svelte is added as a peer dependency so maybe you're right and we can drop the ts-expect-error now?

userquin commented 1 month ago

@mattdavis90 can you check svelte compiler? maybe we can also remove the ts-expect-error (eslint should be complaining about that annotation)

I just noticed in #382 that svelte is added as a peer dependency so maybe you're right and we can drop the ts-expect-error now?

That's why I was asking you ;)

We can even dispatch click events:

imagen

/cc @dominikg @benmccann

sspilleman commented 3 weeks ago

Is this stuck now?

userquin commented 3 weeks ago

@mattdavis90 can you run again pnpm install from root and push lock file again? You don't allow change code to mantainers.

On my local I have this entry modified:

imagen

imagen

mattdavis90 commented 3 weeks ago

Lock file fixed and I've enabled edit from maintainers - apologies