Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't use fetch API when protocol is file: #7706

Closed
wants to merge 1 commit into from
Closed

Don't use fetch API when protocol is file: #7706

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion src/util/ajax.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,12 @@ function makeXMLHttpRequest(requestParameters: RequestParameters, callback: Resp
return { cancel: () => xhr.abort() };
}

const makeRequest = window.fetch && window.Request && window.AbortController ? makeFetchRequest : makeXMLHttpRequest;
const makeRequest = window.fetch &&
window.Request &&
window.AbortController &&
window.location.protocol !== 'file:' ?
makeFetchRequest :
makeXMLHttpRequest;
Copy link
Contributor

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 that makeRequest 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.

Copy link
Author

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 ?

Copy link
Author

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.

Copy link
Member

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.

Copy link
Contributor

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.

Copy link
Contributor

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.


export const getJSON = function(requestParameters: RequestParameters, callback: ResponseCallback<Object>): Cancelable {
return makeRequest(extend(requestParameters, { type: 'json' }), callback);
Expand Down