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

Can't turn off astro:assets for svg #8333

Closed
1 task done
elevatebart opened this issue Aug 31, 2023 · 5 comments · Fixed by #8353
Closed
1 task done

Can't turn off astro:assets for svg #8333

elevatebart opened this issue Aug 31, 2023 · 5 comments · Fixed by #8353
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: assets Related to the Assets feature (scope)

Comments

@elevatebart
Copy link
Contributor

Astro Info

Astro version            v3.0.4
Package manager          yarn
Platform                 darwin
Architecture             arm64
Adapter                  Couldn't determine.
Integrations             @astrojs/sitemap, @astrojs/vue, @astrojs/tailwind

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When I use vite-svg-loader to inline svg in my vue components with the following model, I instead get the astro:assets optimized object.

<script lang="ts" setup>
import Chrome from './Browsers/Chrome.svg?component'
import Firefox from './Browsers/Firefox.svg?component'
</script>
<template>
  <div style="display:flex;flex-gap:4px;">
    <Chrome />
    <Firefox />
  </div>
</template>

What's the expected result?

I would expect the <Chrome/> and <Firefox/> to be vue components instead of ImageMetadata.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-qscvf3?file=src%2Fcomponents%2FTest.vue

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Aug 31, 2023
@elevatebart
Copy link
Contributor Author

elevatebart commented Aug 31, 2023

Seeing the code linked below, I can think of 3 ways I could fix it:

if (rawRE.test(id) || urlRE.test(id)) {
return;
}
const cleanedUrl = removeQueryString(id);
if (/\.(jpeg|jpg|png|tiff|webp|gif|svg)$/.test(cleanedUrl)) {

  1. Let any request that has a query string go to the next plugin. Since the documentation does not say anything about query params, I assume there are none. Why does astro clean the query?
  2. Check where the query is coming from, check the importer. If it is in an .astro file, force it all the time like we do today. If it is a framework file, respect the query. This would keep the current behavior with astro files but avoid messing with the vite config for other items.
  3. Add a config function to ignore certain patterns:
export default defineConfig({
  image: {
    // I am not proud of the name of this config, we should find a better one.
    assetsProcessPredicate(query, importer){
      if (importer.endsWith('.vue') && query.endsWith('.svg?component')) {
        return false
      }
      return true
    },
  },
})

@natemoo-re natemoo-re added feat: assets Related to the Assets feature (scope) - P3: minor bug An edge case that only affects very specific usage (priority) labels Aug 31, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Aug 31, 2023
@natemoo-re
Copy link
Member

This seems valid!

Unfortunately the API you suggested isn't really feasible due to design limitations.

Maybe we need astro:assets to run after other plugins to ensure that other plugins can claim a certain format first? Or we should not remove query params and only support importing .svg files directly.

@Princesseuh
Copy link
Member

The reason we removed query params is to make sure we don't match API endpoints with query params that ended with file extensions, but I think it would be safe to always skip when there's query params?

@elevatebart
Copy link
Contributor Author

@Princesseuh @natemoo-re I can have a PR skipping every import that has query params ready in a few. What do you think?

@Princesseuh
Copy link
Member

@Princesseuh @natemoo-re I can have a PR skipping every import that has query params ready in a few. What do you think?

That seems perfectly fine to me! Thinking about it I can't think of an instance where it would break it. We have some tests around this too that should hopefully prove me wrong if needs be

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: assets Related to the Assets feature (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants