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

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

Closed
floorish opened this issue Feb 11, 2022 · 8 comments
Closed

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

floorish opened this issue Feb 11, 2022 · 8 comments

Comments

@floorish
Copy link
Contributor

floorish commented Feb 11, 2022

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

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

Sure, I'll have a go at it

@floorish
Copy link
Contributor Author

floorish commented Feb 16, 2022

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`));
  1. 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`));
  1. 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: floorish@182bb75

@jpkleemans
Copy link
Owner

jpkleemans commented Feb 17, 2022

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

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

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

No worries, thanks for your replies.

I've added a pr with your suggestion: #42
Had to do a bit of magic in vite.config.js to get testing to work, but should be good now.

@jpkleemans
Copy link
Owner

Added in v3.2.0

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