-
Notifications
You must be signed in to change notification settings - Fork 16
Description
Hi there,
First of all: Fantastic library! ❤️ I've been using it extensively at work 😃
Secondly: I think there is an issue with loading SVGs from cache. It has to do with the following lines of code:
react-svgmt/src/svg-injector.js
Lines 99 to 107 in 0f7ad16
| var loadSvg = function(url, callback) { | |
| if (svgCache[url] !== undefined) { | |
| if (svgCache[url] instanceof SVGSVGElement) { | |
| // We already have it in cache, so use it | |
| callback(cloneSvg(svgCache[url])); | |
| } else { | |
| // We don't have it in cache yet, but we are loading it, so queue this request | |
| queueRequest(url, callback); | |
| } |
The instance of check always returns false. In fact, I don't think that the SVGSVGElement class even exists (could not find it anywhere).
On top of that, the else block seems to do nothing. It just queues the request, but does not actually perform it. That's a problem because that means when re-requesting a URL, the current SVG gets unmounted, but the new one does not get mounted and the screen remains blank.
This is probably left over from transitioning from react-samy-svg to react-svgmt? Git blame brings me here: https://github.com/hugozap/react-svgmt/blame/0f7ad16521bf1070ff9b2e4bc68ecc1eb6b2a005/src/svg-injector.js#L101
I think it's fine to drop the if-else logic and just check if the url is in the cache:
if (svgCache[url] !== undefined) {
// We already have it in cache, so use it
callback(cloneSvg(svgCache[url]));
}Let me know your thoughts/insights!
Cheers,
Finn