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

Correctly handle case where fake XHR returns invalid XML #50

Merged
merged 1 commit into from Jun 1, 2018
Merged

Correctly handle case where fake XHR returns invalid XML #50

merged 1 commit into from Jun 1, 2018

Conversation

m90
Copy link
Contributor

@m90 m90 commented May 26, 2018

When DOMParser fails to parse a XML response, correctly set
responseXML to null. Also, use the JSDOM implementation of
DOMParser when running the test suite as xmldom would not
handle error cases in a correct manner.

Solves #25. It looks like the xmldom parser does not create a <parsererror> node correctly, so it cannot be used in the tests. Since JSDOM also knows about DOMParser it in the meantime I would guess it's ok to use it for doing that instead as it's a dependency anyways.

I'm also sorry for all the package-lock noise, I'm on 6.1.0 and didn't do much but uninstalling xmldom

When DOMParser fails to parse a XML response, correctly set
responseXML to null. Also, use the JSDOM implementation of
DOMParser when running the test suite as xmldom would not
handle error cases in a correct manner.
}
var xmlDoc = new window.ActiveXObject("Microsoft.XMLDOM");
xmlDoc.async = "false";
xmlDoc.loadXML(text);
return xmlDoc;
return xmlDoc.parseError.errorCode !== 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what MSDN says should happen, but I have no way of testing if that is actually correct unfortunately.

Copy link
Contributor Author

@m90 m90 May 26, 2018

Choose a reason for hiding this comment

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

Just got my hands on IE11 and it seems like this actually works as advertised. Alternatively it looks like the loadXML call returns true or false depending on if parsing worked, but since the above is what's documented, I'd stick to it.

Copy link
Contributor

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

Thank you! Looks fine.
LGTM👍 👎

@fatso83 fatso83 merged commit d182803 into sinonjs:master Jun 1, 2018
@fatso83
Copy link
Contributor

fatso83 commented Jun 2, 2018

Seems to break for some. Could you have a look at #54?

@bafolts
Copy link

bafolts commented Jun 2, 2018

I am using xmldom in my testing suite along with nise. Prior to this commit my tests would run fine. Seems like the xmldom issue was known.

@m90
Copy link
Contributor Author

m90 commented Jun 2, 2018

Whoops, that is unfortunate, sorry for the inconvenience @bafolts.

So what's happening is the following: in order to fix #25 it was needed to somehow find out if there is a parsererror node in the document that is being returned as it seems to be the way browsers expect you to handle this. xmldom does not implement this behavior in the way it is specced (it prepends some string instead), so it's more or less impossible to do it with xmldom, which is also why this PR started using JSDOMs DOMParser instead (which works like a browser's DOMParser). I could have put more thought into swapping that out, but apparently I didn't.

The question is now how do we fix situations like this:

  • undo the change and live with FakeXMLHttpRequest falsly sets responseXML #25 instead
  • try to make the parsererror detection resilient enough to work with broken implementations like xmldom and possibly return incorrect values in these cases

Let me know what you think @fatso83 so I can open a PR for this.

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.

None yet

3 participants