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

feat: make SVG type override work #12043

Closed
wants to merge 1 commit into from

Conversation

sapphi-red
Copy link
Member

Description

As reported at #9455 (comment), #9488 wasn't working.

This PR implements in a different way from #9488.

You can try this out at https://stackblitz.com/edit/vitejs-vite-kgv2he?file=src%2Fvite-env.d.ts&terminal=dev

Additional context


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

Before submitting the PR, please make sure you do the following

  • Read the Contributing Guidelines.
  • Read the Pull Request Guidelines and follow the PR Title Convention.
  • Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
  • Ideally, include relevant tests that fail without this PR but pass with it.

@sapphi-red sapphi-red added the p2-nice-to-have Not breaking anything but nice to have (priority) label Feb 13, 2023
@bluwy
Copy link
Member

bluwy commented Feb 13, 2023

I feel like this should be supported in TypeScript instead. Otherwise this introduces a new API and only works with default exports of .svg only.

@sapphi-red
Copy link
Member Author

It would be ideal if TypeScript supports this. But it's a difficult one and I guess it will take a while.
microsoft/TypeScript#31894, microsoft/TypeScript#36146

As *.svg is often the only one that people wants to override, I think it would be nice to have this.
An alternative is to suggest people to import svgs with foo.svg?component and use declare module '*.svg?component'.

@miskoune
Copy link

Thanks for the reactivity on this!

IMHO I think we're trying to solve a problem that doesn't depend on Vite directly.

It's the SVG transformation plugins that should carry the override of types (since they transform the rendering of SVG into components).

And indeed, TypeScript does not yet (unfortunately) allow types overriding on external dependencies.

@sapphi-red , I think that your suggestion to use precise syntax when using SVG as components is the best compromise.

Plugins like vite-plugin-svgr should update the types exposed to handle component SVG in that way.

Note: the vite-svg-loader plugin already does this!


So maybe we should only add as a "tip" the suggestion to add a specific type if the plugin doesn't already handle that in its exported types?

declare module '*.svg?component' {
 ....
}

@patak-dev
Copy link
Member

Something I like about encouraging the use of .svg?component (or even .svg?js) is that it doesn't change the semantics established by Vite and that could generate confusion when switching projects. I think it is a good idea to follow the conventions Vite introduced as much as possible. It would still be good to fix this issue though.

@ArnaudBarre
Copy link
Member

I personally think this is something that should be added by the plugin. This is what I do for my SVG plugin: https://github.com/ArnaudBarre/vite-plugin-fast-react-svg (I've never been a fan of this weird reference thing in dts files)

@patak-dev I understand the idea behind .svg?component, but in a React project, when you can import SVG as a react component, this become the default so this is easier to use. And when you switch project, probably you're switching to another react project that also you a SVG transformer.

@patak-dev
Copy link
Member

Good point, probably for React projects it is more common to be aligned to react-land more than to vite-land regarding the treatment of .svg files

@sapphi-red
Copy link
Member Author

Ah, so type referencing before vite/client works.
https://stackblitz.com/edit/vitejs-vite-7xwuyv?file=src%2Fmain.ts&terminal=dev

I'll update the document to suggest this way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-nice-to-have Not breaking anything but nice to have (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants