Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements #17

Merged
merged 9 commits into from
Feb 7, 2017
Merged

Conversation

emirotin
Copy link
Contributor

@emirotin emirotin commented Feb 2, 2017

This overlaps with #16. Please feel free to reject if you find that approach better fitting your goals, or let me know if you want me to update it after merging that PR.

Basically, I've been following two goals:

  1. mock requests with unpredictable query params (and decided to implement regexes which are not elegant at all, but universal)
  2. have a convenient parsed query to only reply to the requests with specific GET params

index.js Outdated
* @returns {exports}
*/
reset: function() {
MockXMLHttpRequest.handlers = [];
Copy link
Owner

Choose a reason for hiding this comment

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

Can this just proxy to lib/MockXMLHttpRequest.js's reset() rather than re-implementing the logic

Copy link
Owner

Choose a reason for hiding this comment

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

While you're at it can you update teardown() + setup() to reuse it too. ty

index.js Outdated
if (arguments.length === 3) {
matcher = function(req) {
Copy link
Owner

Choose a reason for hiding this comment

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

RegEx. Neat feature!

@@ -68,6 +75,9 @@ function MockXMLHttpRequest() {
this.reset();
this._eventListeners = [];
this.timeout = 0;
// some libraries (like Mixpanel) use the presence of this field to check if XHR is properly supported
Copy link
Owner

Choose a reason for hiding this comment

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

👍

},
"devDependencies": {
"browserify": "^11.0.1",
"browserify": "^14.0.0",
Copy link
Owner

Choose a reason for hiding this comment

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

ty ty

* Get/set the parsed URL query
* @returns {Object}
*/
MockRequest.prototype.query = function() {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like how the query data will be duplicated between .url() and .query().... but I can see how its useful having it accessible as a map

test/index.js Outdated
xhr.send();
})
});

Copy link
Owner

Choose a reason for hiding this comment

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

could you do another test for handlers not matching?

@jameslnewell
Copy link
Owner

Thanks for taking the time and effort to submit this PR! I've got a few minor comments/requests above.

@jameslnewell
Copy link
Owner

jameslnewell commented Feb 3, 2017

Oh, and can you update the doco to reflect the possibility of using a regexp. and the changelog. thanks!

@andrewjtait
Copy link

I was just looking at how to add this feature, great to see it already in progress! I've tested this branch with my project and it works as I'd expect - thanks @emirotin and @boggan.

@emirotin
Copy link
Contributor Author

emirotin commented Feb 6, 2017

Updated

@jameslnewell
Copy link
Owner

💄 Beautiful, thanks! Doesn't look like any breaking changes, version 1.8.0?

@jameslnewell jameslnewell merged commit 2ca8770 into jameslnewell:master Feb 7, 2017
@emirotin
Copy link
Contributor Author

emirotin commented Feb 7, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants