poppa / sveltekit-svg

SvelteKit plugin that makes it possible to import SVG files as Svelte components, inline SVG code or urls
MIT License
235 stars 23 forks source link

Simplify library usage for common usage #47

Closed jerrygreen closed 1 year ago

jerrygreen commented 1 year ago

Instead of this:

<script>
import Logo from "./logo.svg?component";
</script>

<Logo />

I personally expect a more traditional look:

<script>
import Logo from "./logo.svg";
</script>

<Logo />

In addition to better aesthetics, it won't require these hacks with import "@poppanator/sveltekit-svg/dist/svg" in app.d.ts

poppa commented 1 year ago

There's a note about the reason why it is the way it is: https://github.com/poppa/sveltekit-svg/tree/main#svelte-usage

jerrygreen commented 1 year ago

Vite ships a type definition for *.svg which states that import Comp from './file.svg returns a string

Wait you mean it's impossible to override? That's not normal.

If impossible, maybe it's better to contact to vite developers.

But I'm not sure what do we need from them. Just remove the definition?

Even though I'm not sure about an exact way how to solve it from both sides to be satisfied, there should be a way.

poppa commented 1 year ago

I'm not saying its impossible. Its a hassle and it's darn hard to get working reliably. Stuff like this can break when some package is updated and what not, so the solution I'm using in this project is just to make it work 100% of the time.

Nothing is stopping you from trying to override the Vite definition yourself in your own project.

And if you by any chance come up with a solution that will work properly, I'm the first to accept a PR 🤗

jerrygreen commented 1 year ago

After some researching, there's the gist:

So to simplify user experience, it is possible to generate some global.d.ts or svg.d.ts or whatever the name is... Create it in user folder when user first runs vite dev, with triple-slash contents like this:

/// <reference types="@poppanator/sveltekit-svg/dist/svg" />
/// <reference types="vite/client" />

I believe there's a hook in rollup/vite for plugins which allows that. Something like buildStart.

And in @poppanator/sveltekit-svg/dist/svg.d.ts we should have this:

declare module '*.svg'

Instead of our current declaration (or keep both, for backward-compatibility):

declare module '*.svg?component'

But in order for this to work, it's also needed that typescript would read this user svg.d.ts somehow.

I don't really like the idea to change user tsconfig.json, but it seems it's the only way? I'm not sure.

That's the current state of things. I may or may not add the PR, but what do you think about this in general?

UPD. By changing tsconfig.json I meant we need to deeply merge this change (if tsconfig.json exists):

{ "include": ["svg.d.ts"] }
poppa commented 1 year ago

I truly appreciate your effort and research, but the solution is, imho, a hack and those you should stay far away from in library code. Hacks are fine in application code, but you easily shoot both your feet off as soon as you introduce them into libraries.

The cost vs. gain ratio for this is way too poor.

jerrygreen commented 1 year ago

The cost vs gain ratio too poor? You mean the cost of time to spend for this hack, in order to get some convenience?

Because this would tell me that you like the hack in overall. But this contradicts with your first paragraph, therefore I don’t understand why you wrote the second.

Btw I’ve already seen libraries did both of these things, generate a file for typescript with triple-slash notation, and adding to «include» section some of their files. So nothing too special, objectively speaking.

poppa commented 1 year ago

The cost of added complexity to gain very little convenience, is what I'm talking about.

It's irrelevant to me whether other libraries does this or not. This can potentially make the maintainability of the library way harder, since you introduce moving targets, and I'm not interested in that. If other library authors are fine with that, well that's up to them and I fully appreciate that.