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

Fix indexInArray to use the correct array object #434

Closed
wants to merge 1 commit into from

Conversation

prashn64
Copy link
Contributor

@prashn64 prashn64 commented Mar 5, 2014

No description provided.

@kate2753
Copy link
Contributor

kate2753 commented Mar 5, 2014

This change looks good. Few other considerations:

Should we also update line 182 to check that arr is in fact Array?

Why did this not fail any unit tests. Should we add more?

@kate2753
Copy link
Contributor

kate2753 commented Mar 5, 2014

Oh, hmm we don't run our unit tests in IE8 as part of Travis CI, that's why this was not caught.

I wonder if there is a way we can catch issues like this.

@prashn64
Copy link
Contributor Author

prashn64 commented Mar 5, 2014

I'd like to keep line 182 the same, because if we look at these two calls, they should return essentially identical values, including errors:

var fakeArray = {};
fakeArray.indexOf(1); // should throw TypeError: Object # has no method 'indexOf'
dust.indexInArray(fakeArray, 1); // should throw TypeError: Object # has no method 'indexOf'

This actually would not have been caught by a unit test, so I'm now wondering the best way to add that. However, is it as easy as just running travis in IE 8 to test it?

@kate2753
Copy link
Contributor

kate2753 commented Mar 5, 2014

Fair enough. Let's keep 182 as is.

AFAIK Travis does not support testing in browsers. Testing we do right now is all ran in node, including PhantomJS which runs on node.

We could look into integrating with BrowserStack, or other services like this, to run our unit tests in all supported browsers.

We should probably add more unit tests to test the fix in this PR, since there are none that caught the issue.

@prashn64
Copy link
Contributor Author

prashn64 commented Mar 5, 2014

Agreed and I'm also just going to patch backwards again as well, since right now in IE8, it'll spit out every debug message.

@prashn64
Copy link
Contributor Author

prashn64 commented Mar 6, 2014

Please see PR #435

@prashn64 prashn64 closed this Mar 6, 2014
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.

2 participants