Patch Function#call and Function#apply together, more robust than single fix.
#347
Conversation
Xotic750
force-pushed
the
apply
branch
3 times, most recently
from
628a052
to
9561855
Compare
|
I am guess that you will want so more tests added for |
es5-shim.js
Outdated
|
|
||
| if (shouldPatchCallApply) { | ||
| // Constant. ES3 maximum array length. | ||
| var MAX_ARRAY_LENGTH = 4294967295; |
There was a problem hiding this comment.
how is this calculated? Is it the same in every ES3 browser, or does it vary?
There was a problem hiding this comment.
Math.pow(2, 32) - 1. This is the maximum value that any ES3 environment that I have tested supports (I don't know if it is in the spec somewhere). Then in ES5 it changes to Number.MAX_SAFE_INTEGER for arrays and strings (this is in the spec as far I remember). Arrays seem to follow this but strings still seem to be limited to 65k characters, it is a similar story with arguments, I don't think any environment supports more than 65k args (and many support much less). So this seemed a sensible number to choose to check if it was array-like, the smallest max array length? We could set it to Number.MAX_SAFE_INTEGER, but this patch is only going to be applied on ES3 environments, I don't know, I am in two minds about what it should be?
There was a problem hiding this comment.
I have removed this, well the use of it. Need to make sure and remove the code.
|
Since adding the apply tests, there are 4 failures on IE8 that I am not sure about and need to find out why they happen. |
Xotic750
force-pushed
the
apply
branch
3 times, most recently
from
1b567ad
to
68a17a4
Compare
|
Just 1 IE8 failing test remaining. |
Xotic750
force-pushed
the
apply
branch
2 times, most recently
from
3027d01
to
db3d107
Compare
|
IE8 is now without fail. IE7 also tested without fail. Opera 11 and 12 tested ok. Need to test other ES3 environments that I don't have access to. |
| // Boxed string access bug fix. | ||
| if (splitString && to_string.call(argsArray) === '[object String]') { | ||
| // `str_split` is safe to use here, no known issue at present. | ||
| argsArray = call.call(str_split, argsArray, ''); |
There was a problem hiding this comment.
there's already a call-bound strSplit btw, you should just be able to do strSplit(argsArray, '')
There was a problem hiding this comment.
ah wait, i see that it's defined farther down.
I wonder if we should do all the call-bindings before patching Function#call?
There was a problem hiding this comment.
I don't think it will make any difference, when call is patched then bind patch is in play and it will use the patched call. When call is not patched then there is also a native bind. Totally up to you and how you feel about it, do it before or later. The important thing is to not use any bound functions within the patch itself otherwise you end up with a circular condition.
|
Is there is anything more that you require for this patch (except a rebase)? |
|
In general this one makes me very nervous, so I'm intending to let it sit and marinate for awhile. I'll want to extensively test it across many browsers. I'm also curious to see if there's a performance impact on the browsers this patches, and what it might be. |
|
No problem, just say if there are any changes that you want. |
Xotic750
force-pushed
the
apply
branch
2 times, most recently
from
5d390b1
to
a31f499
Compare
… single fix. Ref: es-shims#304 Changed some code back as mentioned in comments. Changed more code as per comments Changed more code as per comments. Changed some variable names to better reflect comments. Fixed missed invocation. Added some comments and code changes as discussed. [tests] Remove unneeded jshint comment. [Tests] use the preferred it/skip pattern for this strict mode test. es-shims#345 (comment) And some other cleanup Added `arguments` expectations to tests. Add tests for `Object#toString` of typed arrays and Symbols, if they exist. Added note about typed array tests. Fix `hasToStringTagRegExpBug` Removed RegExp and Array bug detection as can not test, possible Opera 9. Fixed missing `force` on `defineProperties` that caused the patch to not be applied on IE<9. Fixed `Uint8ClampedArray` tests for Opera 11 and IE10 that don't have it. Removed offending test that was moved to detection, but forgotten. Avoid all possibilities of `call` calling `call`. Do not pass `undefined` argument, we know IE<9 has unfixable bug. Final cleanup (hopeully) Port over work from `apply` fix Move code so that it is specific to the fix. Robustness, move before bind. Remove `Array#slice` tests. Add notes about `eval` and `apply` avoidance.
|
@Xotic750 can you please check the "allow edits" checkbox on the RHS of the PR? |
callRef: #304applyRef: #99Original
callonly fix: #345