Add proxy handler to original XHR #18

AlehVasilyeu · 2017-02-14T16:49:11Z

Hi, here is pull requests that covers issue #11, also additionally slightly changed building.

AlehVasilyeu · 2017-02-14T17:27:20Z

Seems checks failed due to old node 0.10 (one of modules uses arrow functions)? Is it ok to have such old node? :)

AlehVasilyeu · 2017-02-14T17:34:40Z

Also I have idea, why do not put dist into package also? User by default will not build package.

jameslnewell

Hi @AlehVasilyeu. Thanks for your PR!

Unless we need to bump the major version kets keep supporting the same versions of node, and support for arrow functions in the browser isn't quite there I believe so unless we add a transpilation step I don't think we can use arrow functions.

Yep sure, create a UMD build.

jameslnewell · 2017-02-14T19:28:34Z

index.js

-        // otherwise assume the url is a string
-        return url === reqUrl;
+      function match(string, pattern) {
+        return new RegExp(pattern).test(string);


This is a breaking change and I don't think it is quite correct because this code will turn everything into a RegExp XXXGETXXX will be match GET.

Adding the ability to use RegExp to match the method is a great idea 👍 , but like the previous version of the code, please make the RegExp opt-in.

e.g.

function match(string, pattern) { if (pattern instanceof RegExp) { return pattern.test(string); } else { return string === pattern; } }

Good point, will fix it.

jameslnewell · 2017-02-14T19:30:07Z

index.js

+var real                    = window.XMLHttpRequest;
+var mock                    = MockXMLHttpRequest;
+
+window.xhrMockProxyHandler  = require('./lib/xhr-mock-proxy-handler');


I was meaning this could be its own npm package

I wasn't sure what you mentioned in issue. So I agree it's better, let's create a separate NPM package for this. Could we refer to it in README after it will be created?

Also maybe do you have ideas how this package should look like? Should it use browserify as well?

And how will it integrated with your package?

If the user is using browserify/webpack they'll just be able to require/import it.

const xhrMock = require('xhr-mock'); const xhrMockProxyHandler = require('xhr-mock-proxy-handler');

If the user is using the library directly in the browser without browserify/webpack we'll need to use browserify/webpack/rollup to generate a UMD.

<script src="path/to/xhr-mock/dist/xhr-mock.min.js"></script> <script src="path/to/xhr-mock-proxy-handler/dist/xhr-mock-proxy-handler.min.js"></script> <script> window.XHRMock; window.XHRMockProxyHandler </script>

Feel free to keep developing the handler as part of this PR so I can review it, then you can move it to its own package right before I merge the PR.

jameslnewell · 2017-02-14T20:26:22Z

lib/MockXMLHttpRequest.js

@@ -158,40 +159,45 @@ MockXMLHttpRequest.prototype.send = function(data) {

    var response = MockXMLHttpRequest.handle(new MockRequest(self));

-    if (response && response instanceof MockResponse) {
+    Promise.resolve(response).then(function(response) {


I'm not sure that this will handle the case where a promise has been returned by a handler without a response i.e. the handler did something async but still couldn't handle the response.

Should it handle that case?

Hm, I think if request wasn't handled by promise it should be rejected. In other case this promise will be resolved, when passed promise resolved as well.

I tried to do postponed call in xhr-mock-proxy-handler using setTimeout with 5 seconds awaiting. And run written tests, tests passed.

If I incorrectly understood you, please correct me.

With the current implementation of MockXMLHttpRequest.handle, handlers must return a falsey value in order to signify they don't want to handle the request.

That means if any of the handlers returns a Promise (which is truthy), and the Promise does not resolve to a response, the MockXMLHttpRequest.handle method won't continue to loop through the other handlers and the xhr-mock will error because no response was returned, even if there was another handler that would have returned a response.

e.g.

function handler(req, res) { return new Promise(function(resolve, reject) { //resolving without a response if we decided to not handle the request setTimeout(resolve, 100); }); }

This feels like an edge case (creating a handler that asynchronously decides not to return a response) but a bug none-the-less. I wonder if we should handle this in the MockXMLHttpRequest.handle method?

e.g.

//untested code: MockXMLHttpRequest.handle = function(request) { var promise = Promise.resolve(); for (var i=0; i<MockXMLHttpRequest.handlers.length; ++i) { promise = promise.then(function(response) { //if one of the previous handlers has already returned a response, use that if (response) { return response; } return Promise.resolve(MockXMLHttpRequest.handlers[i](request, new MockResponse())); }); } return promise; }

@jameslnewell Interesting point but i'm not sure that we need this.

Let's look on this:

/** * Mock a request * @param {string} [method] * @param {string} [url] * @param {Function} fn * @returns {exports} */ mock: function(method, url, fn) { var handler, matcher; if (arguments.length === 3) { function match(string, pattern) { return new RegExp(pattern).test(string); } handler = function(req, res) { if (match(req.method(), method) && match(req.url(), url)) { return fn(req, res); } return false; }; } else { handler = method; } MockXMLHttpRequest.addHandler(handler); },

We already return false in case if request doesn't match. This doesn't matter what is Response (MockResponse or Promise) and our send() should handle this correctly, trigger error event.

Another case, we have mapping for ANY response with just handler as you mentioned above

function handler(req, res) { return new Promise(function(resolve, reject) { //resolving without a response if we decided to not handle the request setTimeout(resolve, 100); }); }

Yes it will return no response and this will trigger error event as well, what is correct for me.

But in my opinion is not correct if XHR MOCK will try to return response from another request handler that was able fulfill it, it's weird.

I think Developer who use this tool should be careful when specifies mocked requests

What if he has mocked several requests for ANY requests but one responses with timeout and another returns immediately. Which one should response to him? First or second? Or maybe developer should be accurate and clean old mocks?

I think should be used first matched handler, and if it returns no response this is responsibility of developer to write correct one.

Anyway I guess current implementation of handlers could be improved, it iterates across all handler to find suitable. What if use Object instead Array? What do you think?

jameslnewell · 2017-02-14T20:27:50Z

package.json

@@ -19,17 +19,22 @@
    "url": "git://github.com/jameslnewell/xhr-mock.git"
  },
  "dependencies": {
+    "browserify": "^11.0.1",


Can browserify and uglify be moved to the dev dependencies please

Ok, i will try to do build into dist before publish. So move dependencies to dev again.

jameslnewell · 2017-02-14T20:30:51Z

package.json

-    "example.build": "mkdir -p build && browserify -r superagent -r ./index.js:xhr-mock -o build/build.js"
+    "build": "mkdir -p dist && browserify -r lie -r ./index.js:xhr-mock -o dist/xhr-mock.js",
+    "build:example": "browserify -r lie -r superagent -r ./index.js:xhr-mock -o example/xhr-mock.js",
+    "minify": "uglifyjs dist/xhr-mock.js --output dist/xhr-mock.min.js",


Are you including this lib via a script tag in the browser? how are you including and requiring it? can you make it spit out a UMD build?

I use selenium, so I inject script into page. I would like to minify it, so it will speedup.

I think superagent required only for examples, not for everyone.

I more meant, how are you requireing the script on the page? same as the xhr-mock examples?

We should probably generate a UMD (browserify --standalone option).

jameslnewell · 2017-02-14T20:31:59Z

lib/xhr-mock-proxy-handler.js

+
+module.exports = function(req, res) {
+    return new Promise(function(resolve, reject) {
+        var xhr = new realXMLHttpRequest();


How portable is directly relying on XMLHttpRequest? Isn't there a reason there's so many wrappers (browser support)?

I wasn't unable to use axios as you recommended, because it access window.XmlHttpRequest, which can be hidden by xhr-mock. I'm afraid that some else wrapper libraries could do same thing :(

This should be enough portable, maybe some old IE could cause problems. But as I understand you mock XMLHttpRequest as well, not some ActiveX objects from old IEs.

If you mean portability on NodeJS there is xmlhttprequest https://www.npmjs.com/package/xmlhttprequest that behaves like regular XMLHttpRequest.

Good points! Lets leave this as-is.

jameslnewell · 2017-02-14T20:33:11Z

package.json

+    "build": "mkdir -p dist && browserify -r lie -r ./index.js:xhr-mock -o dist/xhr-mock.js",
+    "build:example": "browserify -r lie -r superagent -r ./index.js:xhr-mock -o example/xhr-mock.js",
+    "minify": "uglifyjs dist/xhr-mock.js --output dist/xhr-mock.min.js",
+    "postinstall": "npm run build && npm run minify"


prepublish?

jameslnewell · 2017-02-14T20:34:49Z

README.md

@@ -98,9 +98,13 @@ Register a factory function to create mock responses for each PATCH request to a

 Register a factory function to create mock responses for each DELETE request to a specific URL.

-#### .mock(method, url | regex, fn)
+#### .any(fn)


what's your reasoning for creating a new fn (.mock() will already take a fn)

(note: I like the name better than mock!)

This is my fault i missed that .mock() can configure MockXMLHttpRequest to handle any requests.

Because it's a little bit confusing how it was written (I'm not a Javascript adept). Maybe we can leave behavior you desined but create alias method if you like naming.

Yeah, sorry, the docs probably aren't ideal here. Would this be clearer?

.mock(method: String|RegExp, url: String|RegExp, fn: Function) .mock(fn: Function)

Lets leave out the alias for now (I like to avoid creating multiple ways of doing the same thing - I feel it makes it more difficult for the user to figure out which method to use).

niekbosch · 2017-02-18T08:08:52Z

I have read up on this thread. It comes at exactly the right time for me: I started looking for a mocked version of xhr last week and was not able to find exactly what I need. This package (xhr-mock) came closest, with just those things missing that are being handled in this PR: having a build/browserified version directly in the NPM package and the proxy functionality, where unmocked requests get forwarded to the real xhr object.

If I may give my opinion on some topics addressed in this PR discussion:

A build version (available for browsers) directly in the NPM package is great. I don't care too much about the methodology. For the record: I will use this in a Typescript (Angular 2) project where I transpile ES6 modules to something that SystemJS can handle. But having to add a/some <script> tag(s) to my development HTML doc, is also fine.
I like the approach where you implement it in a handler; very extensible. I do feel it is valid to bundle some default handlers (like this proxy handler) with the xhr-mock package though. It gives me more confidence that it will work in future versions of xhr-mock as well. Which is important for me, because as soon as it doesn't, I need to look for something else. I don't mind helping out with open source projects (that's the idea, right?!), but I do not always have the time to invest in development of the dependencies of my projects.
I am not a big fan of how the handler is bundled at this moment. In a browser, I found the only way to use it, is by referencing it through window.xhrMockProxyHandler

If I can help out in any way, please let me know.

niekbosch · 2017-02-28T16:57:04Z

Reposting my comment:

Using 'lie' as a Promise implementation gives me problems when using xhr-mock in an Angular 2 project. It does not work well with zone.js. Just removing the var Promise = require('lie'); lines, make it work nicely.

In node.js, there is a (native) implementation for promises. And most modern browsers seem to have one as well. Is there any particular reason why lie is used?

If not, would it be possible to leave it up to the user (of your library) to load a polyfill? Alternatively, check if one is loaded before loading lie? Something like:

if (undefined === Promise) {
  var Promise = require('lie');
}

AlehVasilyeu · 2017-02-28T17:21:47Z

Hi, guys, sorry I tied up with work, so I had no time to finish PR. I hope to update it this weekend.

@niekbosch I added lie because not all browsers I needed for testing support Promises natively.

Your solution about using existing Promise object instead requiring new is pretty good. I can add it, if you are ok with this solution.

stukennedy · 2017-06-05T11:14:41Z

is there any movement on this? Our dev team is very keen to have this functionality.
Being able to mock out specific end-points as we develop the front-end would be very valuable.

jameslnewell · 2017-06-06T02:30:33Z

I've started work on v2 which will allow promises to be returned from mock handlers when xhr.open() is called with async=true. Once finished, this will enable someone to write a generic proxy handler as discussed in #11.

jameslnewell · 2017-08-30T11:47:05Z

@continuata I've released xhr-mock@next which allows mock handlers to return a Promise, and will allow people to implement proxies. I've added an example to the readme.

jameslnewell · 2018-01-08T02:45:47Z

xhr-mock@2.0.0-preview.11 has a built-in proxy handler. See the docs here.

jameslnewell · 2018-01-08T04:24:21Z

@niekbosch xhr-mock@next now includes an unminified UMD bundle. Check out this CodePen for an example using the UMD.

Note: The UMD bundle isn't documented in README.md and it doesn't contain the previously mentioned proxy handler. Please raise an issue or a PR describing the ideal approach for implementing these tasks. Ty.

alehvasilyeu-epam added 2 commits February 14, 2017 19:43

Add proxy handler to original XHR

a61c167

Merge branch 'master' into feature/xhr-proxy

6059c3c

alehvasilyeu-epam added 2 commits February 14, 2017 20:47

Replace minifier with uglyfy-js

1f45198

Move superagent packaging to build:example

f62c4b3

jameslnewell requested changes Feb 14, 2017

View reviewed changes

jameslnewell mentioned this pull request Feb 26, 2017

Why use lie for Promises #19

Closed

jameslnewell closed this Jun 6, 2017

Add proxy handler to original XHR #18

Add proxy handler to original XHR #18

Conversation

AlehVasilyeu commented Feb 14, 2017

AlehVasilyeu commented Feb 14, 2017

AlehVasilyeu commented Feb 14, 2017

jameslnewell left a 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

AlehVasilyeu Feb 15, 2017 • edited Loading

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

jameslnewell Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

AlehVasilyeu Feb 28, 2017 • edited Loading

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

jameslnewell Feb 15, 2017 • edited Loading

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

jameslnewell Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

niekbosch commented Feb 18, 2017

niekbosch commented Feb 28, 2017

AlehVasilyeu commented Feb 28, 2017 • edited Loading

stukennedy commented Jun 5, 2017

jameslnewell commented Jun 6, 2017

jameslnewell commented Aug 30, 2017

jameslnewell commented Jan 8, 2018

jameslnewell commented Jan 8, 2018 • edited Loading

AlehVasilyeu Feb 15, 2017 •

edited

Loading

jameslnewell Feb 15, 2017 •

edited

Loading

AlehVasilyeu Feb 28, 2017 •

edited

Loading

jameslnewell Feb 15, 2017 •

edited

Loading

jameslnewell Feb 15, 2017 •

edited

Loading

AlehVasilyeu commented Feb 28, 2017 •

edited

Loading

jameslnewell commented Jan 8, 2018 •

edited

Loading