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

Test that Node is a function before using instanceof on it #1180

Merged
merged 1 commit into from
Mar 6, 2014
Merged

Test that Node is a function before using instanceof on it #1180

merged 1 commit into from
Mar 6, 2014

Conversation

evanc
Copy link

@evanc evanc commented Feb 26, 2014

We use React on a page that also uses the Sarissa library (as part of RichFaces). This library defines its own Node object if the browser doesn't already have one (as IE8 does not). This causes the instanceof check in isNode.js to throw an error.

Reduced test: http://coonrod.org/react/react-0.9.0/examples/basic/

This is the "basic" example from React using the unmodified React 0.9.0. I've added the Sarissa library as well as es5-shim.js and es5-sham.js. If you view it in IE8 you will see something like this:

screen shot 2014-02-25 at 6 13 28 pm

and the timer does not time.

I believe you want to always use instanceof with a function anyway so it seems like a good thing to test when you don't know if you have a good Node or not.

@vjeux
Copy link
Contributor

vjeux commented Feb 26, 2014

cc @yungsters

@syranide
Copy link
Contributor

Seeing as it can only be instanceof Node if it also is a function, this seems like a good fix regardless.

@zpao zpao added CLA needed and removed CLA needed labels Mar 6, 2014
@zpao
Copy link
Member

zpao commented Mar 6, 2014

Seems reasonable. Though it also sounds like Sarissa should polyfill Node slightly more correctly (maybe throw or warn intentionally when used as a function). Have you filed a bug with them?

@evanc
Copy link
Author

evanc commented Mar 6, 2014

unfortunately it’s not been updated in years, we’re moving away from it but we aren’t able to switch everything all at once.

@zpao
Copy link
Member

zpao commented Mar 6, 2014

Thanks Evan!

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.

4 participants