Skip to content
This repository has been archived by the owner on Jul 30, 2018. It is now read-only.

Make test request using currently active protocol #388

Merged
merged 1 commit into from
Apr 19, 2018

Conversation

jason0x43
Copy link
Member

For the blob test, make the test request using the active protocol rather than always using http. At least IE11 will throw an exception when making an http request from an https domain.

Type: bug

The following has been addressed in the PR:

Description:

Resolves #387

For the blob test, make the test request using the active protocol
rather than always using http. At least IE11 will throw an exception
when making an http request from an https domain.

references dojo#387
@@ -19,7 +19,7 @@ add(
}

const request = new global.XMLHttpRequest();
request.open('GET', 'http://www.google.com', true);
request.open('GET', global.location.protocol + '//www.google.com', true);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to hard-code https since location.protocol is blob: in a Worker created from a Blob.

Copy link

@szwoelf szwoelf Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did my tests also with "https" - to be exact with "https://www.google.com". I'd suggest to use it exactly like in Dojo1 (and possibly add the comment that is also there.)

+               // From dojo1: https://github.com/dojo/dojo/blob/master/request/xhr.js#L37
+               // The URL used here does not have to be reachable as the XHR's `send` method is never called.
+               // It does need to be parsable/resolvable in all cases, so it should be an absolute URL
+               // and it must be an https based URL for Internet Explorer 11 (when https is used).
+               // XMLHttpRequest within a Worker created from a Blob does not support relative URL paths.
+               request.open('GET', 'https://dojotoolkit.org', true);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be better to hard-code https since location.protocol is blob: in a Worker created from a Blob.

Not that I'm really opposed, but why? As the comment in the dojo 1 code points out, it needs to be a parseable URL, and for IE it also must be compatible with the current location (which means https when coming from https in IE11). global.location.protocol works because you're just using the current protocol. Https also works, it just seems kind of arbitrary.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm assuming that blob://www.google.com would give an error.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works; I even tested it in an HTTPS app in IE11. (I actually started out with a protocol relative URL, but Web Workers in FF don't like that.) The test URL doesn't have to be an active endpoint, just something the browser could make a request to.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link

@szwoelf szwoelf Apr 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Thanks.

@dylans dylans added this to the 2.0.0 milestone Apr 10, 2018
@rorticus rorticus merged commit 2c57ad0 into dojo:master Apr 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants