Skip to content

Conversation

ernestohs
Copy link

Bug fix and Refactor

  • Refactor embedImageNode to separately handle HTML and SVG image elements by introducing updateHTMLImageElement and updateSVGImageElement helper functions with proper JSDoc comments.
  • Enhance error handling by invoking options.onImageErrorHandler and ensuring lazy-loaded images are set to eager.
  • Update createImage to set crossOrigin, use async decoding, and improve promise handling for better reliability.
  • Rename variables in embedChildren for clarity.
  • Add tests for svgToDataURL and nodeToDataURL to verify correct data URL conversion.

These changes improve code clarity, reliability, and performance in the image embedding process.

Description

Motivation and Context

This change leverages the native decode() method on image elements to ensure that images are fully processed before rendering. Previously, we encountered issues where the final rendered output was incomplete or inconsistent because the image data hadn't been fully decoded when the source was assigned. By invoking decode(), we:

  1. Guarantee Complete Rendering: The image is fully decoded before it's displayed, eliminating rendering glitches.
  2. Enhance Error Handling: Any decoding errors are caught early, allowing us to manage failures gracefully.
  3. Align with Best Practices: Using decode() follows modern web standards for asynchronous image processing, as detailed in the MDN documentation.

This change started because I was reading these issues, and decided to find the root cause of an issue on my project:

Types of changes

  • 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)
  • 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

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

- Refactor embedImageNode to separately handle HTML and SVG image elements by introducing
  `updateHTMLImageElement` and `updateSVGImageElement` helper functions with proper JSDoc comments.
- Enhance error handling by invoking `options.onImageErrorHandler` and ensuring lazy-loaded images
  are set to eager.
- Update createImage to set crossOrigin, use async decoding, and improve promise handling for
  better reliability.
- Rename variables in embedChildren for clarity.
- Add tests for `svgToDataURL` and `nodeToDataURL` to verify correct data URL conversion.

These changes improve code clarity, reliability, and performance in the image embedding process.
@biiibooo
Copy link
Contributor

biiibooo bot commented Feb 25, 2025

👋 @ernestohs

💖 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.

Copy link

codecov bot commented Feb 25, 2025

Codecov Report

Attention: Patch coverage is 90.47619% with 4 lines in your changes missing coverage. Please review.

Project coverage is 66.50%. Comparing base (d9b2fcf) to head (c9d4b5f).
Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
src/embed-images.ts 84.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master     #504   +/-   ##
=======================================
  Coverage   66.50%   66.50%           
=======================================
  Files          10       10           
  Lines         612      612           
  Branches      150      150           
=======================================
  Hits          407      407           
  Misses        144      144           
  Partials       61       61           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@d3-ksmith
Copy link

I gave this a try, and in FireFox it sometimes results in this error: "DOMException: invalid image request."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants