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(asset): add ?inline and ?no-inline queries to control inlining #15454

Merged
merged 15 commits into from
Nov 4, 2024

Conversation

hugoattal
Copy link
Contributor

@hugoattal hugoattal commented Dec 28, 2023

Fix #15453

Description

Adding ?url when importing a file disable inline

Additional context

Recently, browsers disabled data URL in SVG elements (https://developer.chrome.com/blog/migrate-way-from-data-urls-in-svg-use?hl=en)

Since we can't know if a file will be used in an IMG or a USE tag, this option allow users to mark file as non-inlining.

cf this issue #15453


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, especially the Pull Request Guidelines.
  • 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).
  • Update the corresponding documentation if needed.
  • Ideally, include relevant tests that fail without this PR but pass with it.

Copy link

stackblitz bot commented Dec 28, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@hugoattal
Copy link
Contributor Author

hugoattal commented Dec 28, 2023

Sorry, for the force-push, I wanted to retrigger the CI / Build&Test: node-20, macos-latest (pull_request) that seems to fail randomly.

By the way, the fact that fixture.spec.ts fails on adding a new test file is a bit weird 😅...

@patak-dev patak-dev added the p2-to-be-discussed Enhancement under consideration (priority) label Feb 21, 2024
@piotr-cz
Copy link

piotr-cz commented Mar 8, 2024

I wonder why you didn't reuse the ?url suffix used for Explicit URL Imports?

@hugoattal hugoattal changed the title feat(build): add '?file' suffix to disable inline feat(build): add '?url' suffix to disable inline Aug 27, 2024
@hugoattal hugoattal force-pushed the main branch 2 times, most recently from ba59738 to 1f77e94 Compare August 27, 2024 14:08
@hugoattal hugoattal closed this Aug 27, 2024
@hugoattal hugoattal reopened this Aug 27, 2024
@hugoattal
Copy link
Contributor Author

Sorry for the mess, I'm not used to syncing a fork...

I removed the test file since it seems pretty difficult to isolate the test with the new "shouldInline" function that requires the PluginContext.

I also updated the ?file to ?url as suggested.

This ended up in just a single-line update.

@sapphi-red
Copy link
Member

In the last team meeting, we discussed about this PR. We are open to have this feature as long as the following points are resolved.

  • update the document to reflect this feature
  • add a test
  • add a declare module "*?name" type definition
  • a better naming for the query
    • ?url has a different meaning than non inlining
    • the ideas brought up in the meeting
      • ?inline=true/?inline=false
      • ?no-inline
      • ?separate
  • deduping inlined assets with different queries: If we choose ?inline=true for the query name, then we will have duplicate inlined assets in the output bundle when a user imports an asset that is inlined by default with both ?inline=true and without a query.

@hugoattal hugoattal changed the title feat(build): add '?url' suffix to disable inline feat(build): add '?no-inline' suffix to explicitly disable inlining file Oct 25, 2024
@hugoattal
Copy link
Contributor Author

@sapphi-red I just updated the PR with your recommendations.

I chose the ?no-inline approach, I think it was the most straightforward to understand (and ?inline already exists, so I didn't want to change the way it worked)

I don't think I need to take a special look at deduping since I went with this solution, unless I overlooked something?

Let me know if anything needs to change, thanks again for discussing it!

@codethief
Copy link

codethief commented Oct 27, 2024

@sapphi-red

?url has a different meaning than non inlining

What other meaning does it have then? The docs explicitly state that a ?url import should yield a URL, nothing else. The current behavior of inlining ?url imports opportunistically violates this contract, though. A data URL is not a regular URL. I can't use it in an anchor tag, for instance. I also cannot fire asynchronous requests against it or use it in a <link> tag.

If you want ?url to mean "regular URL or data URL" then I think the docs need to be updated. (And IMO ?url should be renamed to ?url-or-data-url or something.) The current behavior, though, is very surprising.

@hugoattal hugoattal requested a review from bluwy October 28, 2024 17:51
@bluwy bluwy changed the title feat(build): add '?no-inline' suffix to explicitly disable inlining file feat(asset): add ?inline and ?no-inline queries to control inlining Oct 29, 2024
bluwy
bluwy previously approved these changes Oct 31, 2024
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM now. I think the query name looks fine compared to other alternatives.

patak-dev
patak-dev previously approved these changes Oct 31, 2024
@patak-dev patak-dev added this to the 6.0 milestone Oct 31, 2024
@patak-dev
Copy link
Member

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 8cda9f9: Open

suite result latest scheduled
astro failure success
histoire failure success
redwoodjs failure failure
remix failure failure
storybook failure success
sveltekit failure failure
vike failure failure
vite-plugin-vue failure success
vitest failure failure

analogjs, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress, vuepress

@sapphi-red
Copy link
Member

A data URL is not a regular URL. I can't use it in an anchor tag, for instance. I also cannot fire asynchronous requests against it or use it in a <link> tag.

?url only means it's a URL, it does not say it's a URL with http / https protocol. FYI top-level navigation does not work with data URLs for security reasons, but you can still download a data URL file by adding a download attribute. Also fetch supports data URLs (e.g. fetch('data:,Hello%2C%20World%21')).

sapphi-red
sapphi-red previously approved these changes Nov 4, 2024
patak-dev
patak-dev previously approved these changes Nov 4, 2024
@bluwy bluwy dismissed stale reviews from patak-dev and sapphi-red via 91b1889 November 4, 2024 14:44
@bluwy bluwy enabled auto-merge (squash) November 4, 2024 14:44
@bluwy bluwy merged commit 9162172 into vitejs:main Nov 4, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2-to-be-discussed Enhancement under consideration (priority)
Projects
Archived in project
6 participants