Add nativeDataView check to isDataView #2986

colingm · 2023-08-23T16:39:59Z

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 #2985

jgonggrijp

Thanks for taking the extra step to address this yourself!

You can test this as follows:

Define new tests for the hypothetical scenario that DataView has been overridden, which only run when this is the case.
Run the test suite to verify that your new tests don't run.
Adjust the guards on the existing tests related to DataView, so they don't run if DataView has been overridden.
Run the test suite to verify that the latter tests still run.
Write a new module inside the test/ directory that actually overrides DataView.
Add the overriding module to test/index.html and the files field in karma.conf.js and karma.conf-sauce.js.
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.
Fiddle with your code as necessary until your new tests pass everywhere.
Remove the overriding module from the test/index.html again.
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.
...?
profit!

jgonggrijp · 2023-08-23T23:41:04Z

modules/_setup.js

@@ -21,7 +21,7 @@ export var push = ArrayProto.push,

 // Modern feature detection.
 export var supportsArrayBuffer = typeof ArrayBuffer !== 'undefined',
-    supportsDataView = typeof DataView !== 'undefined';
+    supportsDataView = typeof DataView !== 'undefined' && /\[native code\]/.test(String(DataView));


Are all JS engines guaranteed to stringify the function in that way? Otherwise, I would prefer that you go for something like typeof DataView === 'function'. That doesn't completely rule out overrides, but at least those would be constructable.

That is a great question and consideration. So here is how far back I have tested based on browser support:

IE9 & older: no DataView support

IE10 & IE11: passes

Safari 8 (couldn't go back to Safari 5.1 where DataView was added): passes

Chrome 26 (couldn't go back to Chrome 9 where DataView was added): passes

Edge 13: passes

Firefox 14 & older: no DataView support

Firefox 15: passes

Opera: now uses Chromium engine and passes, didn't test pre-chromium Opera

As far as I am aware this should show it is the consistent way to do a toString for native code, it always looks like this:

function FUNCTION_NAME(FUNCTION_PARAMS) { [native code] }

I could have used a function type check like you suggested but I was still a little uneasy with this approach. DataView is a fairly common name used by third party tools and I have seen it be overriden a few times. I would be concerned about the case where DataView is overriden where it still has a constructor but it doesn't behave the same way so it doesn't actually know what to do with an ArrayBuffer. That might be a bit overly cautious so I am fine going that route if it makes everyone feel more comfortable with this change though.

You do make a convincing case that [native code] should be at least somewhat portable. Have you also tried it with Node or Deno?

Regardless of the approach, I'm afraid a watertight detection is not possible, anyway. [native code] is easy to fool with window.DataView = window.ArrayBuffer, for example.

Yep I tested them both and they handle it the same. You are correct that it is never going to be a watertight detection, to me it is just what is the most likely to occur. At the very least Sizzle.js uses a variation of the same check quite regularly so I am confident in it being a reasonable enough solution: https://github.com/jquery/sizzle/blob/4194dc464e7381139220e39ceb46c0bdfe12dd59/src/sizzle.js#L142

colingm · 2023-08-24T03:17:47Z

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 · 2023-08-30T14:17:08Z

@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.

jgonggrijp

Thanks! Your overall approach looks good to me.

In the test/index.html, you placed the overrides after the Underscore UMD bundle instead of before it. If your tests work as intended, then this should cause them to fail in IE10 through Edge 13. Do you have the means to test that?

test/overrides.js

test/objects.js

test/index.html

colingm · 2023-09-08T15:21:41Z

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

jgonggrijp

Sorry, a few more details I didn't spot before.

By the way, do you have the means to test the wrong order of the test/index.html in IE or Edge 13?

test/objects.js

jgonggrijp · 2023-09-08T22:03:07Z

test/objects.js

-        assert.ok(_.isEqual(new DataView(u8.buffer), new DataView(i8.buffer)), 'Identical DataViews of different typed arrays are equal');
-        assert.notOk(_.isEqual(new DataView(u8.buffer), new DataView(u16.buffer)), 'Different DataViews with different length are not equal');
-        assert.notOk(_.isEqual(new DataView(u8.buffer), new DataView(u16one.buffer)), 'Different DataViews with different byte data are not equal');
+      var DataViewImpl = typeof NativeDataView !== 'undefined' ? NativeDataView : typeof DataView !== 'undefined' ? DataView : undefined; // Browser could override DataView


Please turn the new comment into a full sentence and put it on a separate line above the code it annotates. That keeps it consistent with the literate style of the library.

jgonggrijp · 2023-09-08T22:04:22Z

test/objects.js

-      // Some older browsers support typed arrays but not DataView.
-      if (typeof DataView !== 'undefined') {
-        checkValues['a DataView'] = new DataView(buffer);
+      var DataViewImpl = typeof NativeDataView !== 'undefined' ? NativeDataView : typeof DataView !== 'undefined' ? DataView : undefined; // Browser could override DataView


This is a repetition of line 612. You can lift it to the scope of the IIFE wrapper (but first change the comment as I suggested there).

jgonggrijp · 2023-09-08T22:05:02Z

test/overrides.js

+  }
+
+  var runOverrides = Math.floor(Math.random() * 3) === 0;
+  if (runOverrides) { // Only override browser functions roughly 1/3rd of the time


Please turn this into a standalone comment as well.

colingm · 2023-09-08T23:12:36Z

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 · 2023-09-11T14:27:18Z

@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 · 2023-09-11T15:00:48Z

@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

jgonggrijp

I think your name changes are justified, although I'm not entirely happy with them. I have made a suggestion below for the variable name.

In any case, please remove the docs from your pull request, those should be updated only when we cut a new release. (Are you running the prepublishOnly script as a commit hook?)

Otherwise, I'm happy with your changes!

jgonggrijp · 2023-09-19T15:54:40Z

modules/_stringTagBug.js

+// Also, there are cases where an application can override the native
+// `DataView` object, in cases like that we can't use the constructor
+// safely and should just rely on alternate `DataView` checks
+export var useAlternateDataViewCheck = (


Maybe hasDataViewBug?

yeah I like that name better

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.

This includes a new module that will run about 1 out of every 3 runs that will make sure the browser DataView is replaced with an object. Without the updates to supportsDataView this would fail since stringTagBug would try to call the constructor on it.

colingm · 2023-09-27T13:55:53Z

@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 · 2023-09-27T14:01:47Z

Well done and thanks again!

colingm · 2023-09-27T14:04:02Z

no problem thanks for your guidance along the way!

colingm force-pushed the native-data-view branch from 1be5440 to 0b5d30f Compare August 23, 2023 18:39

jgonggrijp reviewed Aug 24, 2023

View reviewed changes

jgonggrijp requested changes Sep 8, 2023

View reviewed changes

test/overrides.js Outdated Show resolved Hide resolved

test/overrides.js Outdated Show resolved Hide resolved

test/objects.js Outdated Show resolved Hide resolved

test/index.html Outdated Show resolved Hide resolved

colingm force-pushed the native-data-view branch from 1fdae4b to 6967907 Compare September 8, 2023 14:56

jgonggrijp approved these changes Sep 8, 2023

View reviewed changes

jgonggrijp requested changes Sep 8, 2023

View reviewed changes

colingm force-pushed the native-data-view branch from 6967907 to f25a7db Compare September 11, 2023 14:58

jgonggrijp requested changes Sep 19, 2023

View reviewed changes

colingm force-pushed the native-data-view branch from f25a7db to a4a8da3 Compare September 27, 2023 13:42

colingm added 2 commits September 27, 2023 09:52

colingm force-pushed the native-data-view branch from a4a8da3 to dbb5475 Compare September 27, 2023 13:53

jgonggrijp approved these changes Sep 27, 2023

View reviewed changes

jgonggrijp merged commit 0bd9b48 into jashkenas:master Sep 27, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add nativeDataView check to isDataView #2986

Add nativeDataView check to isDataView #2986

colingm commented Aug 23, 2023

jgonggrijp left a comment

jgonggrijp Aug 23, 2023

colingm Aug 24, 2023

jgonggrijp Aug 24, 2023

colingm Aug 25, 2023

colingm commented Aug 24, 2023

colingm commented Aug 30, 2023

jgonggrijp left a comment

colingm commented Sep 8, 2023

jgonggrijp left a comment

jgonggrijp Sep 8, 2023

jgonggrijp Sep 8, 2023

jgonggrijp Sep 8, 2023

colingm commented Sep 8, 2023

colingm commented Sep 11, 2023

colingm commented Sep 11, 2023

jgonggrijp left a comment

jgonggrijp Sep 19, 2023

colingm Sep 27, 2023

colingm commented Sep 27, 2023

jgonggrijp commented Sep 27, 2023

colingm commented Sep 27, 2023

Add nativeDataView check to isDataView #2986

Add nativeDataView check to isDataView #2986

Conversation

colingm commented Aug 23, 2023

jgonggrijp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colingm commented Aug 24, 2023

colingm commented Aug 30, 2023

jgonggrijp left a comment

Choose a reason for hiding this comment

colingm commented Sep 8, 2023

jgonggrijp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colingm commented Sep 8, 2023

colingm commented Sep 11, 2023

colingm commented Sep 11, 2023

jgonggrijp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colingm commented Sep 27, 2023

jgonggrijp commented Sep 27, 2023

colingm commented Sep 27, 2023