Add nativeDataView check to isDataView

[colingm]

The is DataView check relies on DataView being a constructor like it is natively. However, if an application overrides DataView from the window this ends up throwing an error when underscore is included. This here will check if DataView is the native implementation before trying to use the constructor on it. If it is not native it will fallback on the same heuristic used on older IE versions for checking if something is a DataView or not.

I wasn't really sure what to do for tests here since we don't seem to be testing any of the supports or native methods. We still have the tests for isDataView and those seem to pass in a standard browser as well as in the case of you overriding the native data view.

This is to resolve https://github.com/jashkenas/underscore/issues/2985

[colingm]

Thanks for taking the extra step to address this yourself!

You can test this as follows:

1. Define new tests for the hypothetical scenario that `DataView` has been overridden, which only run when this is the case.

2. Run the test suite to verify that your new tests don't run.

3. Adjust the guards on the existing tests related to `DataView`, so they don't run if `DataView` has been overridden.

4. Run the test suite to verify that the latter tests still run.

5. Write a new module inside the `test/` directory that actually overrides `DataView`.

6. Add the overriding module to `test/index.html` and the `files` field in `karma.conf.js` and `karma.conf-sauce.js`.

7. Run the tests; verify that your new tests are included and that the tests that should be excluded are skipped. Try `npm run test-node` and `npm run test-browser` as well as opening the `test/index.html` in all browsers you have installed locally.

8. Fiddle with your code as necessary until your new tests pass everywhere.

9. Remove the overriding module from the `test/index.html` again.

10. Adjust the `karma.conf.js` and the `karma.conf-sauce.js` so that the overriding module is included randomly, once every three runs on average.

11. ...?

12. profit!

Awesome, thanks for the testing suggestion here. I had investigated 1-5 but wasn't sure how much value was in those types of tests in comparison to the new module suggestion you had. I will go ahead and look at adding both types of tests.

[colingm]

@jgonggrijp I have gone ahead and added a module that will override dataview in the browser that makes the tests fails without my corresponding change to supportsDataView. Let me know what you think, sorry it took a bit longer to get back to this than I intended.

[colingm]

@jgonggrijp Okay I think I addressed all of your comments, thanks for that!

[colingm]

Yeah I will make it work to test those with both the wrong order and the right order. Also I will take care of those style things too.

[colingm]

@jgonggrijp I was going through and making those changes and testing on IE when I realized I have created an odd combination sometimes where you would normally have the stringTagBug and you can't rely on the string tag to know if an object is a DataView and you also have overriden the native DataView to be something else. I think I am going to try to rework the check, basically instead of it just being "hasStringTagBug" I would like to change it to more of a check of whether or not you need to use an alternate isDataView check. This will result in the renaming of a few things so after my next commit there might be a little bit more to review.

[colingm]

@jgonggrijp okay I went ahead and pushed the changes to those test files as well as the rework for the DataView check that isolates a bit more the problem. The biggest thing for me right now is the naming structure. It made sense to not leave it as "hasStringTagBug" to me but I am not sure if you agree or agree with the new name for that function so please review and let me know what you think.

Also I went ahead and tested my old version before this rework and then my new version after the rework against IE10 and the new version actually passed all of the tests. The old version meant it would rely on tagTester('DataView') which didn't work in IE10 where as this new version will just use the alternate data view check for IE10 and for overriden DataView

[colingm]

@jgonggrijp Thank you for that last comment, I was also in a place where I wanted to change the name but naming is hard and I wasn't perfectly happy with what I had. I think I have everything taken care of now (no more docs changes). I think I had the docs changes because I had used build so that I could get the minified changes deployed to a site for testing on older browsers and then accidentally committed them.

[jgonggrijp]

Well done and thanks again!

[colingm]

no problem thanks for your guidance along the way!

[jgonggrijp]

Released in 1.13.7. Sorry for the delay!

[colingm]

@jgonggrijp that is alright, thank you for helping get this out there!