fix `fetch API rasies TypeError when argument is URL object` (close #1613) #1623

Farfurix · 2018-05-18T07:06:24Z

fetch supports the following browsers: Chrome, Firefox, Edge, Safari.

References

Changes

Change arguments processing logic in fetch() src/client/sandbox/fetch.js:
If the input argument(args[0]) is not a string and is not a Request object,
we are trying to convert the input argument to string primitive value using String().
This approach helps us simulate the browsers behavior by throwing the 'TypeError' exception
if the input object can not be converted to a primitive value.
Fix "request promise should be rejected on invalid calling (Wrong behaviour for window.fetch request called without parameters #939)" test.
Fix XMLHttpRequest: convert url argument to string before getProxyUrl. Fix test/client/before-test.js checkNativeFunctionCalling.
Fix locationWrapper (href setter, assgin, replace): convert url argument to string.
Fix location property case. Add routes in test/client/config-qunit-server-app.js for iframe loading in IE 11 (location property test)
Fix "xhr.response.url" test: exclude Internet Explorer 11 with 0 assertions.
Remove checkNativeFunctionArgs('open', 'xhrOpen', xhr) test case from windows-test.js because of the url argument string conversion.

Fix "request promise should be rejected on invalid calling (GH-939)" test

Browsers process all of these arguments in fetch without Promise rejection:

var testCases = [
    123,
    function () {
    },
    null
];

Simple example:

var testFetch = function (firstArg) {
    fetch(firstArg)
        .then(function (res) {
            console.log(res.url);
        })
        .catch(function (err) {
            console.log(err);
        });
}

> testFetch(123) 
http://127.0.0.1:8080/123

> testFetch(null) 
http://127.0.0.1:8080/null

> testFetch(function () {} ) 
http://127.0.0.1:8080/function%20()%20%7B%20%20%20%20%7D

Safari processed `fetch()` without `Promise` rejection

> fetch()
http://127.0.0.1:8080/undefined

Browsers process inaccessible `null` and `undefined` arguments without `Promise` rejection

> testFetch(null) 
http://127.0.0.1:8080/null

> testFetch(undefined) 
http://127.0.0.1:8080/undefined

`Object` in the `fetch` first argument

URL:

> testFetch(new URL('http://127.0.0.1:8080/index2.html')) 
http://127.0.0.1:8080/index2.html

Object without toString method:

var firstArg = {
    url: 'http://127.0.0.1:8080/index2.html'
};

> testFetch(firstArg)
http://127.0.0.1:8080/[object%20Object]

Object withtoString method:

var firstArg = {
    url:      'http://127.0.0.1:8080/index2.html',
    toString: function () {
        return this.url;
    }
};

> testFetch(firstArg)
http://127.0.0.1:8080/index2.html

testcafe-build-bot · 2018-05-18T08:21:42Z

❌ Tests for the commit 984020d have failed. See details:

miherlosev · 2018-05-18T14:05:29Z

src/client/sandbox/fetch.js

@@ -53,7 +55,24 @@ export default class FetchSandbox extends SandboxBase {
    }

    static _isValidRequestArgs (args) {
-        return typeof args[0] === 'string' || isFetchRequest(args[0]);
+        if (typeof args[0] === 'string' || isFetchRequest(args[0]))


Bad design. _isValidRequestArgs should not modify the arguments.

Rewrite as the following:

return typeof args[0] === 'string' || isFetchRequest(args[0]) || objectWithToStringMethod;

Move argument processing logic for null, undefined and custom object with toString method to FetchSandbox._processArguments(args).

miherlosev · 2018-05-18T14:25:24Z

test/client/fixtures/sandbox/fetch-test.js

+                        return this.url;
+                    }
+                },
+                expectedUrl: 'https://example.com/some-path'


Add test for object without toString method

testcafe-build-bot · 2018-05-18T15:02:05Z

❌ Tests for the commit 4e699bb have failed. See details:

testcafe-build-bot · 2018-05-21T12:07:09Z

❌ Tests for the commit 931baa9 have failed. See details:

testcafe-build-bot · 2018-05-21T16:29:02Z

✅ Tests for the commit 1029adf have passed. See details:

LavrovArtem · 2018-05-22T08:52:51Z

test/client/fixtures/sandbox/fetch-test.js

+                    });
+            };
+
+            var promises = [];


var cases = [ checkArg({ toString: function () { return {}; } }) ];

testcafe-build-bot · 2018-05-22T09:40:31Z

❌ Tests for the commit ccb2aeb have failed. See details:

testcafe-build-bot · 2018-05-22T12:40:31Z

❌ Tests for the commit f9bed6d have failed. See details:

Farfurix · 2018-05-22T18:57:48Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-05-22T19:27:29Z

❌ Tests for the commit f9bed6d have failed. See details:

testcafe-build-bot · 2018-05-25T07:18:48Z

❌ Tests for the commit 8521d2b have failed. See details:

testcafe-build-bot · 2018-05-25T10:59:26Z

✅ Tests for the commit c7a2e68 have passed. See details:

testcafe-build-bot · 2018-05-25T11:57:49Z

✅ Tests for the commit d02d5b9 have passed. See details:

testcafe-build-bot · 2018-05-28T14:50:40Z

❌ Tests for the commit 9c3493f have failed. See details:

Farfurix · 2018-05-28T17:31:47Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-05-28T18:15:38Z

❌ Tests for the commit 9c3493f have failed. See details:

testcafe-build-bot · 2018-05-29T05:49:17Z

✅ Tests for the commit 864bd6d have passed. See details:

testcafe-build-bot · 2018-05-29T08:24:16Z

❌ Tests for the commit 2d87ed8 have failed. See details:

testcafe-build-bot · 2018-05-29T14:42:52Z

❌ Tests for the commit 7f3566a have failed. See details:

Farfurix · 2018-05-29T17:47:28Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-05-29T18:10:31Z

❌ Tests for the commit 7f3566a have failed. See details:

testcafe-build-bot · 2018-05-30T06:23:46Z

❌ Tests for the commit aa0d6dd have failed. See details:

testcafe-build-bot · 2018-05-30T07:50:01Z

❌ Tests for the commit 33ee438 have failed. See details:

Farfurix · 2018-05-30T07:50:36Z

@testcafe-build-bot \retest

testcafe-build-bot · 2018-05-30T08:17:36Z

✅ Tests for the commit 33ee438 have passed. See details:

miherlosev · 2018-06-04T11:21:37Z

src/client/sandbox/code-instrumentation/location/wrapper.js

@@ -61,6 +61,10 @@ export default class LocationWrapper extends EventEmitter {
            return ensureTrailingSlash(href, locationUrl);
        };
        const getProxiedHref = href => {
+            // NOTE: we must convert url-object to 'string' (GH-1613)


remove this comment

miherlosev · 2018-06-04T11:22:10Z

src/client/sandbox/fetch.js

-        if (inputIsString) {
-            args[0] = getProxyUrl(input);
+        if (!inputIsFetchRequest) {
+            // NOTE: we must convert url-object to 'string' (GH-1613)


remove this comment

miherlosev · 2018-06-04T11:27:33Z

src/client/sandbox/xhr.js

+            const url         = arguments[1];
+            const urlIsString = typeof input === 'string';
+
+            // NOTE: we must convert url-object to 'string' (GH-1613)


remove this comment

testcafe-build-bot · 2018-06-04T14:06:54Z

❌ Tests for the commit 7975445 have failed. See details:

…window-test.js

testcafe-build-bot · 2018-06-06T06:16:52Z

✅ Tests for the commit 7f37db8 have passed. See details.

miherlosev · 2018-06-06T08:02:31Z

src/client/sandbox/fetch.js

                return nativeMethods.fetch.apply(this);

-            if (!FetchSandbox._requestIsValid(args))
-                return sandbox.window.Promise.reject(new TypeError());
+            // NOTE: If the input argument(args[0]) is not a `string` and is not a `Request` object,


Remove this comment

miherlosev · 2018-06-06T08:04:35Z

src/client/sandbox/fetch.js


-            FetchSandbox._processArguments(args);
+            if (!FetchSandbox._sameOriginCheck(args))


Move FetchSandbox._sameOriginCheck to the FetchSandbox._processArguments

miherlosev · 2018-06-06T08:07:25Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

@@ -192,6 +192,204 @@ test('special pages (GH-339)', function () {
    destLocation.forceLocation(storedForcedLocation);
 });

+test('should process some url types in locationWrapper (href, replace, assign) (GH-1613)', function () {


test('different url types for locationWrapper methods (href, replace, assign) (GH-1613)'

miherlosev · 2018-06-06T08:10:18Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+    });
+});
+
+test('should throwing an error on invalid url-object in locationWrapper (href, replace, assign) (GH-1613)', function () {


test('throwing errors on calling locationWrapper methods (href, replace, assign) with invalid arguments

miherlosev · 2018-06-06T08:11:19Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+    }
+});
+
+test('should process some url types in the "location" property (GH-1613)', function () {


test('different url types for "location" property (GH-1613)')

miherlosev · 2018-06-06T08:13:30Z

test/client/fixtures/sandbox/code-instrumentation/location-test.js

+        });
+    };
+
+    var checkIframesLocation = function (nativeIframeUrl, processedIframeUrl) {


var checkLocationAssignment = ...

miherlosev · 2018-06-06T08:14:14Z

test/client/fixtures/sandbox/fetch-test.js

@@ -62,6 +62,52 @@ if (window.fetch) {
            });
    });

+    test('should process some argument types (GH-1613)', function () {


different url types for 'fetch' (GH-1613)

miherlosev · 2018-06-06T08:15:25Z

test/client/fixtures/sandbox/xhr-test.js

@@ -76,6 +78,66 @@ test('toString, instanceof, constructor and static properties', function () {
    strictEqual(XMLHttpRequest.DONE, nativeMethods.XMLHttpRequest.DONE);
 });

+test('overridden "open" function should process some url argument types before the native "XMLHttpRequest.open" method calling (GH-1613)', function () {


test('different url types for xhr.open method (GH-1613)')

miherlosev · 2018-06-06T08:16:09Z

test/client/fixtures/sandbox/xhr-test.js

+    nativeMethods.xhrOpen = storedNativeXhrOpen;
+});
+
+test('should throwing an error on invalid calling "open" (GH-1613)', function () {


test('should throwing an error on invalid calling "open" method (GH-1613)'

miherlosev · 2018-06-06T08:17:34Z

test/client/fixtures/sandbox/xhr-test.js

-            start();
-        }
+// NOTE: IE11 doesn't support 'XMLHttpRequest.responseURL'
+if (browserUtils.isIE11) {


Why does this condition need?

testcafe-build-bot · 2018-06-06T11:48:30Z

✅ Tests for the commit b35f554 have passed. See details.

…evExpress#1613) (DevExpress#1623) * fix `fetch API rasies TypeError when argument is URL object` * requested changes, fix Safari case * refactoring * requested changes * requested changes * requested changes: fix tests * add URL test case * xhr, location.href, location.assign, location.replace cases * requested changes: fix xhr.open test, remove unnecessary testcase in window-test.js * requested changes: refactor xhr.open test * fix location property case * try to fix tests * try to fix tests * fix location property test * try to fix location property test * try to fix location property test * fix double quotation in location property test * requested changes * requested changes * restore xhr.responseURL test

Farfurix requested a review from LavrovArtem May 18, 2018 07:06

miherlosev requested changes May 18, 2018

View reviewed changes

LavrovArtem suggested changes May 22, 2018

View reviewed changes

Farfurix changed the title ~~[WIP] fix fetch API rasies TypeError when argument is URL object (close #1613)~~ fix fetch API rasies TypeError when argument is URL object (close #1613) May 30, 2018

LavrovArtem approved these changes May 30, 2018

View reviewed changes

miherlosev requested changes Jun 4, 2018

View reviewed changes

Farfurix added 17 commits June 6, 2018 08:44

requested changes, fix Safari case

8b4af35

refactoring

2ef9ed4

requested changes

f4652a6

requested changes

616c4c6

requested changes: fix tests

f6d08bd

add URL test case

c6fb517

xhr, location.href, location.assign, location.replace cases

a4373e8

requested changes: fix xhr.open test, remove unnecessary testcase in …

0ef4f91

…window-test.js

requested changes: refactor xhr.open test

91f4500

fix location property case

60ca2de

try to fix tests

e3bd00a

try to fix tests

be5dabf

fix location property test

9424b0e

try to fix location property test

5e82cb1

try to fix location property test

b96c6d7

fix double quotation in location property test

e25ccb6

requested changes

7f37db8

miherlosev requested changes Jun 6, 2018

View reviewed changes

requested changes

d8a8f85

AndreyBelym closed this Jun 6, 2018

AndreyBelym reopened this Jun 6, 2018

AndreyBelym closed this Jun 6, 2018

AndreyBelym reopened this Jun 6, 2018

restore xhr.responseURL test

b35f554

miherlosev approved these changes Jun 6, 2018

View reviewed changes

miherlosev merged commit 0bb81c9 into DevExpress:master Jun 6, 2018

Farfurix deleted the i1613 branch June 9, 2018 09:43


		FetchSandbox._processArguments(args);
		if (!FetchSandbox._sameOriginCheck(args))

fix fetch API rasies TypeError when argument is URL object (close #1613) #1623

fix fetch API rasies TypeError when argument is URL object (close #1613) #1623

Conversation

Farfurix commented May 18, 2018 • edited Loading

References

Changes

Fix "request promise should be rejected on invalid calling (GH-939)" test

Safari processed fetch() without Promise rejection

Browsers process inaccessible null and undefined arguments without Promise rejection

Object in the fetch first argument

testcafe-build-bot commented May 18, 2018

miherlosev May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented May 18, 2018

testcafe-build-bot commented May 21, 2018

testcafe-build-bot commented May 21, 2018

Choose a reason for hiding this comment

testcafe-build-bot commented May 22, 2018

testcafe-build-bot commented May 22, 2018

Farfurix commented May 22, 2018

testcafe-build-bot commented May 22, 2018

testcafe-build-bot commented May 25, 2018

testcafe-build-bot commented May 25, 2018

testcafe-build-bot commented May 25, 2018

testcafe-build-bot commented May 28, 2018

Farfurix commented May 28, 2018

testcafe-build-bot commented May 28, 2018

testcafe-build-bot commented May 29, 2018

testcafe-build-bot commented May 29, 2018

testcafe-build-bot commented May 29, 2018

Farfurix commented May 29, 2018

testcafe-build-bot commented May 29, 2018

testcafe-build-bot commented May 30, 2018

testcafe-build-bot commented May 30, 2018

Farfurix commented May 30, 2018

testcafe-build-bot commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Jun 4, 2018

testcafe-build-bot commented Jun 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

testcafe-build-bot commented Jun 6, 2018

fix `fetch API rasies TypeError when argument is URL object` (close #1613) #1623

fix `fetch API rasies TypeError when argument is URL object` (close #1613) #1623

Farfurix commented May 18, 2018 •

edited

Loading

Safari processed `fetch()` without `Promise` rejection

Browsers process inaccessible `null` and `undefined` arguments without `Promise` rejection

`Object` in the `fetch` first argument

miherlosev May 18, 2018 •

edited

Loading