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

Use absolute URL in XHR feature tests #320

Closed
msssk opened this issue Apr 11, 2017 · 6 comments · Fixed by #351
Closed

Use absolute URL in XHR feature tests #320

msssk opened this issue Apr 11, 2017 · 6 comments · Fixed by #351
Assignees
Milestone

Comments

@msssk
Copy link
Contributor

msssk commented Apr 11, 2017

Bug / Enhancement

Any dojo/has test operating on an XMLHttpRequest instance that might be run from within a Web Worker generated from a blob should use an absolute URL (see https://bugs.dojotoolkit.org/ticket/18998).

This is most likely relevant in has('blob'):

request.open('GET', '/foo', true);

A unit test similar to the one in Dojo 1 PR 260 should be included.

@dylans dylans added this to the 2017.04 milestone Apr 11, 2017
@agubler agubler self-assigned this Apr 12, 2017
@agubler agubler added the bug label Apr 12, 2017
@agubler agubler removed their assignment Apr 12, 2017
@agubler
Copy link
Member

agubler commented Apr 12, 2017

The has check also needs to use global.XMLHttpRequest over XMLHttpRequest, something like this:

add('blob', function () {
	if (!has('xhr2')) {
		return false;
	}

	const request = new global.XMLHttpRequest();
	request.open('GET', 'http://google.com', true);
	request.responseType = 'blob';
	request.abort();
	return request.responseType === 'blob';
});

@agubler agubler added the beta2 label Apr 12, 2017
@kitsonk
Copy link
Member

kitsonk commented Apr 12, 2017

We should also consider the ordering/impact/effect of global on this. In particular when using jsdom under NodeJS, a lot of these sniffs are going to be effected depending on what environment is returned in global.

Currently @dojo/core/global has window having precedence over global meaning that the global context returned will be the JSDom window when using the loadJsdom method of loading the DOM.

But the dojo/shim#80 proposes to change the order, so that global would be returned first over window in anticipation of the introduction of global as part of the ES standards.

So even using global might not be the most reliable sniff for this, and instead we should use global.window && global.window.XMLHttpRequest() (as this would be valid in all environments, irrespective of the order of precedence as window.window is valid). The XHR sniffs should be updated for this as well.

@agubler
Copy link
Member

agubler commented Apr 12, 2017

@kitsonk That's a fair point but would mean changing more than the currently broken sniff (means we'd need to change more than blob has check).

Do you think that warrants its own issue?

@kitsonk
Copy link
Member

kitsonk commented Apr 12, 2017

Well, for this sniff, the safest thing would still be global.window.XMLHttpRequest and it likely means the has('xhr') and has('xhr2') sniff should be made "safe" as well. I would see that as being addressed by this issue of fixing the proper sniffing, IMO.

As far as larger impacts on ordering and generally properly sniffing window features. Yes, that should be a separate issue.

@kitsonk
Copy link
Member

kitsonk commented Apr 12, 2017

dojo/has#48 covers the proper use of global and window in feature tests and utilising browser features.

@agubler
Copy link
Member

agubler commented Apr 12, 2017

The fix has gone in but this still needs to have tests added.

@dylans dylans modified the milestones: 2017.04, 2017.05 Apr 29, 2017
@eheasley eheasley modified the milestones: 2017.05, 2017.06 Jun 6, 2017
@dylans dylans modified the milestones: 2017.06, 2017.07 Jul 4, 2017
@dylans dylans modified the milestones: 2017.07, 2017.08 Aug 4, 2017
rorticus added a commit to rorticus/core that referenced this issue Aug 9, 2017
rorticus added a commit to rorticus/core that referenced this issue Aug 11, 2017
rorticus added a commit to rorticus/core that referenced this issue Aug 14, 2017
@kitsonk kitsonk modified the milestones: 2017.08, 2017.09 Sep 4, 2017
rorticus added a commit that referenced this issue Sep 7, 2017
* Adding tests for web workers, issue #320

* Adjusting test to work with postMessage, issue #320

* Relying on host browser rather than window object
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants