-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add nativeDataView check to isDataView #2986
Conversation
1be5440
to
0b5d30f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ifDataView
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 overridesDataView
. - Add the overriding module to
test/index.html
and thefiles
field inkarma.conf.js
andkarma.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
andnpm run test-browser
as well as opening thetest/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 thekarma.conf-sauce.js
so that the overriding module is included randomly, once every three runs on average. - ...?
- profit!
modules/_setup.js
Outdated
@@ -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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
1fdae4b
to
6967907
Compare
@jgonggrijp Okay I think I addressed all of your comments, thanks for that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
test/objects.js
Outdated
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
test/overrides.js
Outdated
} | ||
|
||
var runOverrides = Math.floor(Math.random() * 3) === 0; | ||
if (runOverrides) { // Only override browser functions roughly 1/3rd of the time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please turn this into a standalone comment as well.
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. |
@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. |
6967907
to
f25a7db
Compare
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
modules/_stringTagBug.js
Outdated
// 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 = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe hasDataViewBug
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I like that name better
f25a7db
to
a4a8da3
Compare
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.
a4a8da3
to
dbb5475
Compare
@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. |
Well done and thanks again! |
no problem thanks for your guidance along the way! |
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