fix: make external svg files referred to in use tags work

strindhaug commented 10 months ago

Description

I noticed that when using html-to-image on our website it crashed with mysterious errors about invalid queries. I realised that the code does not handle external files in use-tags in an inline SVG.

Motivation and Context

Fix for https://github.com/bubkoo/html-to-image/issues/425 (and also implicitly for https://github.com/bubkoo/html-to-image/issues/392 )

Also in order for the test case to be useful I reduced the allowed number of incorrect pixels from 100 to 2, thus somewhat addressing https://github.com/bubkoo/html-to-image/issues/426 it could possibly now be a little too sensitive on other platforms such as Windows that use a different font rendering, but at least the new test and all the old tests ran om my Mac.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to change)
[ ] Enhancement (changes that improvement of current feature or performance)
[ ] Refactoring (changes that neither fixes a bug nor adds a feature)
[x] Test Case (changes that add missing tests or correct existing tests)
[ ] Code style optimization (changes that do not affect the meaning of the code)
[ ] Docs (changes that only update documentation)
[ ] Chore (changes that don't modify src or test files)

Self Check before Merge

[x] My code follows the code style of this project. (I think I have, I have at least linted and added several comments explaining my thought process)
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.
[x] I have read the CONTRIBUTING document.
[x] I have added tests to cover my changes.
[x] All new and existing tests passed.

vivcat[bot] commented 10 months ago

👋 @strindhaug

💖 Thanks for opening this pull request! 💖

Please follow the contributing guidelines. And we use semantic commit messages to streamline the release process.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add graph.scale() method
docs: graph.getShortestPath is now available

Things that will help get your PR across the finish line:

Follow the TypeScript coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

codecov[bot] commented 10 months ago

Codecov Report

Patch coverage: 95.45% and no project coverage change.

Comparison is base (b751cbf) 62.93% compared to head (225e357) 62.93%. Report is 2 commits behind head on master.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #427 +/- ## ======================================= Coverage 62.93% 62.93% ======================================= Files 10 10 Lines 580 580 Branches 143 143 ======================================= Hits 365 365 Misses 151 151 Partials 64 64 ``` | [Files Changed](https://app.codecov.io/gh/bubkoo/html-to-image/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96) | Coverage Δ | | |---|---|---| | [src/clone-node.ts](https://app.codecov.io/gh/bubkoo/html-to-image/pull/427?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=%E5%B4%96#diff-c3JjL2Nsb25lLW5vZGUudHM=) | `71.42% <95.45%> (ø)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

strindhaug commented 10 months ago

Btw, apparently I have a newer pnpm version because when I initialised the repo the pnpm-lock.yaml was modified automatically. Looking at the diff it's mostly updating the pnpm version number and reordered slightly. I chose not to commit this change to avoid lots of pointless autogenerated diffs.

strindhaug commented 10 months ago

BTW: Looks like 🏷️　Label(Patch Size) / run (pull_request_target) is misconfigured, it seems to try to pull data about this PR but from my account where this PR is not present (and if I had made a PR to myself for some reason, it probably would not have the id 427).

I see several other PR's have this check failing.

Tilesto commented 8 months ago

Please, merge it((

ZhongxuYang commented 10 hours ago

Thanks to this PR, making a patch saved me.

bubkoo / html-to-image