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

fix(postcss-svgo): skip non-SVG urls #1133

Merged
merged 1 commit into from May 25, 2021
Merged

fix(postcss-svgo): skip non-SVG urls #1133

merged 1 commit into from May 25, 2021

Conversation

ludofischer
Copy link
Collaborator

Fix #1050

test(
'should skip non-SVG urls',
passthroughCSS(`@font-face {
src: url("../example/dfds.woff2") format("woff2"),
Copy link
Member

Choose a reason for hiding this comment

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

Can we add test with relative SVG URL?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. I think if everything works as expected the plugin should ignore it. The issue is in fact that it should ignore all URLs expect data:image. The plugin cannot transform a SVG which is just linked but not inlined.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2021

Codecov Report

Merging #1133 (8e3e9da) into master (069c424) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1133      +/-   ##
==========================================
+ Coverage   96.36%   96.42%   +0.05%     
==========================================
  Files         115      115              
  Lines        3576     3578       +2     
  Branches     1051     1050       -1     
==========================================
+ Hits         3446     3450       +4     
+ Misses        121      119       -2     
  Partials        9        9              
Impacted Files Coverage Δ
packages/postcss-svgo/src/index.js 100.00% <100.00%> (+3.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 069c424...8e3e9da. Read the comment docs.

@ludofischer
Copy link
Collaborator Author

Updated. I suspect this is another consequence of #1034 When we removed is-svg we started processing too many things that were not SVGs.

@alexander-akait
Copy link
Member

We can check it using \.svg extension, don't forget about ?something=bar in query strings

Fix #1050

Ensure it never starts processing an URL before making
sure it is an inine SVG.
@ludofischer
Copy link
Collaborator Author

I've added another test with svg in a URL parameter. It should be good because it's already testing with these regexes

const dataURI = /data:image\/svg\+xml(;((charset=)?utf-8|base64))?,/i; 
const dataURIBase64 = /data:image\/svg\+xml;base64,/i;

The issue was that it new URL before testing with the regex (because before there was also the is-svg test.

@ludofischer ludofischer merged commit 0ff1716 into master May 25, 2021
@ludofischer ludofischer deleted the fix-svgo branch May 25, 2021 16:07
@internalsystemerror
Copy link

@ludofischer @alexander-akait thank you both

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.

None yet

4 participants