Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change doesn't fix the issue. It switches to XHR when the document is loaded from a file URL. However, in Safari, Chrome and Firefox, using the Fetch API to load resources works even if the document was loaded from a file URL. The way I read the original Cordova issue, the fact that the document is loaded from a file URL doesn't matter, it's only the original of the resource that is loaded with
makeRequest
that matters. This means thatmakeRequest
needs to be a function that discriminates between file and non-file requests and chooses XHR/Fetch (if available) at request time, and not at initialization time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it does fix the issue in cordova (I did test it in a cordova app) but you're right that you might still be able to use the Fetch API to load https: resources (but not http:) in this context.
As I understand it, you do prefer a more specific solution at the expense of increased code complexity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should add that indeed it doesn't fix the issue if in cordova context and the page shown in the webview is not loaded over file: (not the typical use case).
I assumed in this case you shouldn't be loading file: resources but I might miss some use cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kkaefer as far as I understand, the assumption here is that it's very unlikely when setting up a Cordova app to load the main HTML file externally but resources locally, so this PR will generally fix the use case of using local resources while remaining simple and minimal. Thus it might make sense to accept it in this form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That may be, but it's still incorrect, and the proposed correct solution is viable and terse as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, a potential application would be to load the main UI from a website, but load e.g. fonts locally since they never change.