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

Simplify library usage for common usage #47

Closed
jerrygreen opened this issue Aug 16, 2023 · 7 comments
Closed

Simplify library usage for common usage #47

jerrygreen opened this issue Aug 16, 2023 · 7 comments

Comments

@jerrygreen
Copy link

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
Copy link
Owner

poppa commented Aug 16, 2023

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
Copy link
Author

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
Copy link
Owner

poppa commented Aug 16, 2023

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
Copy link
Author

jerrygreen commented Aug 16, 2023

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
Copy link
Owner

poppa commented Aug 17, 2023

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
Copy link
Author

jerrygreen commented Aug 17, 2023

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
Copy link
Owner

poppa commented Aug 17, 2023

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.

@poppa poppa closed this as completed Aug 18, 2023
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

No branches or pull requests

2 participants