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.

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

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

Closed
wants to merge 1 commit into from

Conversation

gpiffault
Copy link

Launch Checklist

Closes #7603

This PR adds the condition that the protocol is not file: to use the fetch API

  • briefly describe the changes in this PR
  • manually test the debug page

@mourner mourner requested a review from kkaefer December 19, 2018 09:28
window.AbortController &&
window.location.protocol !== 'file:' ?
makeFetchRequest :
makeXMLHttpRequest;
Copy link
Member

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
Member

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
Member

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.

@kkaefer
Copy link
Member

kkaefer commented Jan 30, 2019

Alternative implementation landed in #7818

@kkaefer kkaefer closed this Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants