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

Add Partial XMLHttpResponse 2 Support #60

Merged
merged 4 commits into from Nov 27, 2014
Merged

Add Partial XMLHttpResponse 2 Support #60

merged 4 commits into from Nov 27, 2014

Conversation

@albertyw
Copy link
Contributor

@albertyw albertyw commented May 4, 2014

I added in the response property that was introduced in XMLHttpResponse2. This should help to address #49.

I'm not sure how to run tests though, other than waiting for Travis.

@albertyw
Copy link
Contributor Author

@albertyw albertyw commented May 5, 2014

This is backwards incompatible with being able to do jasmine.Ajax.requests.mostRecent().response since response is now an object. It should be jasmine.Ajax.requests.mostRecent().makeResponse.

@tdg5
Copy link
Contributor

@tdg5 tdg5 commented Jun 12, 2014

I took a stab at adding to this PR. My goal was to make it so response doesn't need to be explicitly set. It handles a responseType of text or json well, but throws an error stating that response must be explicitly set for responseType of arraybuffer, blob, or document. There may be a way to handle these responseTypes better, but seems like some of the logic is internal to the browser, so took the short road for now.

The additional commit can be found here: tdg5@a93edf5

Seems like phantomjs tests are still struggling in Travis.

albertyw and others added 3 commits May 4, 2014
Conflicts:
	spec/javascripts/mock-ajax-toplevel-spec.js
@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Jun 13, 2014

@tdg5 I rebased your commit onto my pull request. I've totally forgotten the code I wrote a month ago though. I've also given you write privilege to my fork if you wish to develop on there.

@tdg5
Copy link
Contributor

@tdg5 tdg5 commented Jun 17, 2014

Is there an official opinion of how this might be handled? What I've added feels half baked, but I haven't come up with a better solution.

@slackersoft
Copy link
Member

@slackersoft slackersoft commented Sep 7, 2014

We'd really like to get XHR2 support in, but we've been trying to figure out if there is a way to do that without changing the interface for jasmine ajax. @jboyens and I have discussed this off-and-on in person, trying to figure out if there is a way to do something like this without changing the interface for jasmine ajax.

One thought was to have XHR2 support be an option you'd turn on, and only at that point would the
interface change so response behaved differently.

Another option might be to have response behave differently depending on whether any arguments were passed to it. So with no args, return the XHR2 response, but otherwise, do the current behavior.

Neither of these really seems awesome, but the other options effectively mean we have to break the interface again after we just released a 2.0.

In addition, there's been some more refactoring of the code, and all of the individual pieces are now pulled out into their own files for easier testing, and then combined later, so this PR will need to be rebased before we can accept it.

@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Sep 8, 2014

I would be happy to refactor and clean up the code in this pull request but it would be nice to have a high level decision about how to proceed.

I agree with your options and I personally think the second option sounds better because turning on or off XHR2 support is not something you can do in a browser anyways. The second option can also be made safer by checking for tell-tale XHR2 calls, such as setting xhr.responseType(type) though it wouldn't be foolproof.

@awk
Copy link

@awk awk commented Sep 13, 2014

The problem with changing the behaviour of response is that in XHR2 it's a property not a method. So when code under test uses response it's always :

parse(xhr.response)

and never

parse(xhr.response())

Which sadly makes the trick of looking at the number of arguments a non-starter :-( Whilst the mode switch might be annoying it looks to be the only solution right now without breaking API compatibility.

@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Sep 13, 2014

@awk agreed that's a problem. I come from a python background so in python I would be using the @property decorator which would run a function without parentheses. It looks like javascript doesn't do well with overloading functions or calling functions without using parens though.

@slackersoft
Copy link
Member

@slackersoft slackersoft commented Sep 14, 2014

Sounds like we're just going to need to make an interface change. I think I'd prefer something like respondWith instead of makeResponse, but either way is fine.

@awk
Copy link

@awk awk commented Sep 14, 2014

I'd prefer respondWith too - it's more in keeping with the language of Jasmine I think.

@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Sep 14, 2014

I took a crack at rebasing this pull request and fixing conflicts but I'm honestly a little lost. It seems there's been a refactor that I've missed. For example, why are there duplicate RequestStubs in requestStub.js and mock-ajax.js?

@slackersoft
Copy link
Member

@slackersoft slackersoft commented Sep 14, 2014

lib/mock-ajax.js is now generated by concatenating everything in src. Make your changes to the files in src and run grunt concat to generate mock-ajax.js

@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Sep 14, 2014

shouldn't mock-ajax.js not be tracked in git then?

@slackersoft
Copy link
Member

@slackersoft slackersoft commented Sep 15, 2014

mock-ajax.js is tracked in git to allow users to run from master on github when there are commits that haven't been officially released.

@slackersoft slackersoft added the waiting label Nov 1, 2014
slackersoft added a commit that referenced this pull request Nov 17, 2014
Starting on #49 #60 #89
@slackersoft slackersoft merged commit 63bd205 into jasmine:master Nov 27, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@albertyw
Copy link
Contributor Author

@albertyw albertyw commented Jan 26, 2015

Thanks for the release, @slackersoft !

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

Successfully merging this pull request may close these issues.

None yet

4 participants