fix: Normalize URLs on Ionic WKWebViews #228
Conversation
lib/imgcache.js
Outdated
| return (typeof entry.toURL === 'function' ? Helpers.EntryToURL(entry) : entry.toURI()); | ||
| var url = (typeof entry.toURL === 'function' ? Helpers.EntryToURL(entry) : entry.toURI()); | ||
| // Ionic has a URL normalization method for WKWebView (#223) | ||
| var normalizedUrl = window.Ionic && window.Ionic.normalizeURL && window.Ionic.normalizeURL(url) || url; |
There was a problem hiding this comment.
You can use WKWebView without having Ionic in your stack. Thus, the solution should not rely on its existence. And if your problem is Ionic specific, it shouldn't be solved here.
There was a problem hiding this comment.
Yes, but it will only use Ionic if it exists, in order to be compatible with Ionic. Without this piece of code, images don't load at all in Ionics webview unless everyone adds similar code themselves.
So any non-Ionic users will not see any difference, and any Ionic users will have it work out of the box. Seems helpful to me. 🤷♀️
There was a problem hiding this comment.
I believe the result could be even more confusing to users because the behavior will be completely different depending on what dependencies are used in the project and part of the behavior of the plugin will be defined elsewhere.
You also just noticed yourself that this integration is apparently bound to break when the Ionic team makes changes to their API. Maintainers of this code should not have to care about Ionic IMHO.
If you're in Angular land, it should be trivial to sanitize your URLs before passing them to imgcache.js, we do so as well in our code. If that is not possible or undesirable, why not pull in the same sanitation code Ionic uses and replicate it in the Helper section of the plugin? Then it would work for everyone.
Anyway, I'm not the maintainer of this project. Just wanted to share my thoughts and possibly reach a better solution :)
|
I rebased it and updated to support v2 of ionics webviews. They changed the naming of the function around, and it is now required for android as well. https://github.com/ionic-team/cordova-plugin-ionic-webview#migrating-to-2x |
|
Hey, that's odd! What version of cordova-file-plugin are you using? Since v1.1.0 toURL shouldn't return
Edit: Even more odd is that I cannot see the comment from @phillipplum 🤷♂️ |
|
I use the plugin-version 6.0.1. Edit: Strange @Maistho :/ |
|
I'm closing this PR since it probably shouldn't be merged. If this is still an issue I think adding some documenation in the README.md would be preferable. |
Normalizes URLs on ionics wkwebview.
Recommended by the Ionic devs as a reasonable solution.
ionic-team/cordova-plugin-ionic-webview#76 (comment)
fixes #223
Let me know if there's anything else I've missed!