jpkleemans / vite-svg-loader

Vite plugin to load SVG files as Vue components
MIT License
557 stars 59 forks source link

Changing the default import option when no query params are given #41

Closed floorish closed 2 years ago

floorish commented 2 years ago

Currently an svg without a query param is imported as a Component. Is it possible to change that to the ?url behavior instead?

Generally, I'm importing svgs as <img/> in Vue templates/CSS, and occasionally use the ?component query param to get a component instead. But all those <img/> need a ?url query param, otherwise they're not loaded when using Vite build.

It's a bit weird to add these ?url params everywhere, e.g.:

.some-class {
    background: url('/src/assets/arrow.svg?url');
}
<img src="/src/assets/arrow.svg?url">

Particularly because the default for other assets is without query params:

.some-class {
    background: url('/src/assets/arrow.png');
}
<img src="/src/assets/arrow.png">

And if you accidentally forget the ?url param you'll only notice missing assets when using vite build. When using vite dev the assets are still shown correctly, so they're hard to spot when developing.

Changing the default would be a breaking change, so instead a plugin config option that sets the default would be nice to have.

jpkleemans commented 2 years ago

Hi, thanks for the proposal. I agree having a 'default loading behavior' config option would be nice. Could you create a PR for that?

floorish commented 2 years ago

Sure, I'll have a go at it

floorish commented 2 years ago

This turned out to be a bit trickier than expected. I'm not too familiar with rollup plugins, so couple questions:

resolveid is currently not used at all. It should be resolveId (note case sensitive). But when I change it to the correct case building the examples doesn't work anymore and Vite outputs errors: Error: Could not load ./assets/test.svg (imported by src/App.vue): ENOENT: no such file or directory, open './assets/test.svg' Perhaps this needs to be a separate issue report, but I'm not sure what the resolveId function should do :)

When I set the default to url instead of component via the config, it looks like dynamic imports won't work properly if it contains variables.

  1. Here the plugin thinks the default is url, so you'll end up with both a circle.js and circle.svg, the js imports the svg via an url. Not expected and will fail because it is not a component:

    const name = "circle";
    const Async = defineAsyncComponent(() => import(`./assets/${name}.svg`));
  2. This doesn't reach the load function at all (somehow rollup checks if circe.svg?component file actually exists?):

    const name = "circle";
    const Async = defineAsyncComponent(() => import(`./assets/${name}.svg?component`));
  3. This works fine, reaches the load method and transforms to circle.js:

    const Async = defineAsyncComponent(() => import(`./assets/circle.svg?component`));

Is this an acceptable tradeoff?

Final question: the project uses cypress to test the examples. When testing the proposed change the config of vite.config.js needs to be adjusted. Would a full copy of the vue example folder with adjusted config & App.vue be fine with you? Or is there a better way to test this?

See wip here: https://github.com/floorish/vite-svg-loader/commit/182bb7569bde90ebe94770f1cf32f746da1b2423

jpkleemans commented 2 years ago

Hi, maybe something like this will work in the load method:

async load(id) {
  const [path, query] = id.split('?', 2)

  const importType = query || defaultImport || 'component'

  if (importType === 'url') {
    return // Use default svg loader
  }

  if (importType === 'raw') {
    return `export default ${JSON.stringify(svg)}`
  }

  if (importType === 'component') {
    // Component compilation code
  }
}
floorish commented 2 years ago

That's just a different style, right? Perhaps I'm missing something, but that doesn't address the concerns mentioned in the previous post:

I'll gladly submit a pr if you don't care about these things.

jpkleemans commented 2 years ago

Hi, thanks for your questions. I misinterpreted your first post.

resolveid is non-functional at the moment

I see, we can remove that function then. Thanks.

async imports cannot have variables when defaultImport == 'url'

Yes, that is indeed a rollup specific problem. It also doesn't work for other assets:

const name = 'logo'
const url = import(`./assets/${name}.png?url`) // Error: Unknown variable dynamic import: ./assets/logo.png?url

how to properly test this?

Hmm, good question. I don't really like copying the whole folder. Can you maybe use env variables to update the config?

floorish commented 2 years ago

No worries, thanks for your replies.

I've added a pr with your suggestion: https://github.com/jpkleemans/vite-svg-loader/pull/42 Had to do a bit of magic in vite.config.js to get testing to work, but should be good now.

jpkleemans commented 2 years ago

Added in v3.2.0