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): inline svg in dev if within limit #18581

Merged
merged 4 commits into from
Nov 6, 2024
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Nov 5, 2024

Description

close #18034

As discussed before, due to quote handling difference between SVG url paths and inlined SVG data url, we decided to align the inlined behaviour from build to dev so that users don't get broken SVGs after deploying.

This PR adds the code to do the inline checks in dev for SVG assets. The downside I guess is that we're relying on build config in dev, which we usually try to avoid, but we may have to make an exception here.


NOTES:

  1. I also handled a bug where inlined urls in dev (which is supported here and since feat(asset): add ?inline and ?no-inline queries to control inlining #15454) are incorrectly injected with ?t= for HMR invalidation. Since data URLs are the entire content itself, we don't need to inject it for them.
  2. I refactored the conditions in shouldInline a bit so that:
    • .html and .svg checks are done earlier
    • Separate the build-only logic within it under a buildPluginContext parameter

patak-dev
patak-dev previously approved these changes Nov 5, 2024
Copy link
Member

@patak-dev patak-dev left a comment

Choose a reason for hiding this comment

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

I think it would be good in the future to move the build.assetsInlineLimit to root (where we also have now assetsInclude). But it sounds good to me to move forward with the PR as is, and do this later on.

I think maybe we could have top-level { assets: { include, inlineLimit }

@patak-dev patak-dev added this to the 6.0 milestone Nov 5, 2024
@bluwy
Copy link
Member Author

bluwy commented Nov 6, 2024

I think it would be good in the future to move the build.assetsInlineLimit to root (where we also have now assetsInclude). But it sounds good to me to move forward with the PR as is, and do this later on.

I think maybe we could have top-level { assets: { include, inlineLimit }

I'm not too sure if we need to go there yet since that could imply the inlining will consistently happen in dev too, but we're currently only doing it for SVGs. But definitely something to watch for though if we're using more build options in dev.

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

Successfully merging this pull request may close these issues.

Got wrong url when importing some svg files
3 participants