dojo / core

:rocket: Dojo 2 - language helpers and utilities.
http://dojo.io
Other
213 stars 58 forks source link

Use absolute URL in XHR feature tests #320

Closed msssk closed 7 years ago

msssk commented 7 years ago

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'): https://github.com/dojo/core/blob/8baeca295d709fc005141015b33321de5911ee93/src/has.ts#L20

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

agubler commented 7 years ago

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';
});
kitsonk commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

agubler commented 7 years ago

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