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

Implement method override check without instanceof #32

Merged
merged 1 commit into from
May 8, 2018

Conversation

rayd
Copy link
Contributor

@rayd rayd commented May 8, 2018

This is a proposed fix for #8:
In order to support making assertions against elements that exist within
another iframe, we cannot use instanceof to check if we should
override the Assertion method. This fails because window.HTMLElement !== otherWindow.HTMLElement. Instead use the nodeType property to
check if we're dealing with an element, or for the NodeList check use
the class name.

In order to support making assertions against elements that exist within
another iframe, we cannot use `instanceof` to check if we should
override the Assertion method. This fails because `window.HTMLElement
!== otherWindow.HTMLElement`. Instead use the `nodeType` property to
check if we're dealing with an element, or for the `NodeList` check use
the class name.
@nathanboktae
Copy link
Owner

Thanks! I like this implementation. Did you test this with jsdom by chance?

@rayd
Copy link
Contributor Author

rayd commented May 8, 2018

I haven't actually - just with Chrome, Firefox and PhantomJS - any suggestions for testing with jsdom?

@nathanboktae
Copy link
Owner

I did a manual sanity test and it looks fine. I should add another test target or such for jsdom.

@nathanboktae nathanboktae merged commit 828ef55 into nathanboktae:master May 8, 2018
@rayd
Copy link
Contributor Author

rayd commented May 8, 2018

👍 awesome, thank you for being so responsive!

@nathanboktae
Copy link
Owner

No problem! 1.8.0 is released with the improvement. Thanks for the contribution.

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

2 participants