Fix `Function.prototype.call` and tests. #345

Xotic750 · 2015-11-15T19:45:46Z

This is not necessarily ready to pull, but I want for the tests to be run.

Xotic750 · 2015-11-15T19:55:24Z

All seems well so far on Node. Is there somewhere to see the automated browser tests (if there are any)?

ljharb · 2015-11-15T19:58:13Z

es5-shim.js

-// Add semicolon to prevent IIFE from being passed as argument to concatenated code.
-;
+// Added to override editor options
+/*jshint maxparams:false, maxdepth:false, maxstatements:false, maxcomplexity:false,


please revert this - we don't use jshint in this repo.

I know, it was just while in my editor, I will remove stuff that has nothing to do with the fix.

ljharb · 2015-11-15T22:50:12Z

Thanks for bearing with all the comments! Please rebase this on top of latest master as you make your changes as well.

Xotic750 · 2015-11-15T22:51:32Z

Will do. Comments are good, I'm learning. ;)

ljharb · 2015-11-15T23:16:37Z

tests/spec/s-function.js

+                testFn,
+                subject;
+
+            testFn = function () {


we don't use onevar, so please use one var per var

ljharb · 2015-11-17T08:55:55Z

es5-shim.js

+          }
+      }
+      var i = arguments.length;
+      var rest = i ? Array(--i) : [];


please explicitly compare to 0 instead of relying on truthiness - however, i'm not sure why we need Array() at all? There's no performance benefit towards "allocating" slots on a JS array, in fact the opposite - because it initially creates a sparse array.

Ok, I thought it was cheaper than using push, if you have a large number or arguments? Actually in the apply fix, I have removed this completely. Maybe I should port some of that work over to this fix, as things need to be changed slightly for it to work?

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

Xotic750 · 2015-11-17T09:40:19Z

es5-shim.js

 var max = Math.max;
 var min = Math.min;

+// To prevent recursion if `Function#call` or `Function#apply` are patched.


This actually needs to be moved to happen on when patch is applied. I will fix it an rebase later.

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.

Xotic750 · 2015-11-17T10:09:40Z

es5-shim.js

+// Inconsistent `[[Class]]` strings for `window` or `global`.
+
+if (hasToStringTagLegacyArguments) {
+    // To prevent recursion if `Function#call` or `Function#apply` are patched.


This actually seems to have some strange effect on newer environments (when patch is forced) when not used in conjunction with my apply patch. I'm thinking that I may remove it from this this PR. Need to check that I haven't been stupid first though.

Xotic750 · 2015-11-17T15:04:48Z

I am going to remove the Array#slice test from this PR and submit those separately.

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 · 2015-11-17T19:17:20Z

I now have apply working on IE8
https://rawgit.com/Xotic750/es5-shim/apply/tests/index.html
Ref: #99
I feel it is more robust to implement both fixes together. What are your thoughts on how to proceed?

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.

… 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 · 2015-11-19T15:49:57Z

call + apply fix is far more stable.

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

ljharb reviewed Nov 15, 2015
View reviewed changes

ljharb reviewed Nov 17, 2015
View reviewed changes

Xotic750 force-pushed the call branch from fe284ac to b213e67 Compare November 17, 2015 09:20

Xotic750 reviewed Nov 17, 2015
View reviewed changes

Xotic750 force-pushed the call branch from b213e67 to d8f337a Compare November 17, 2015 09:48

Xotic750 reviewed Nov 17, 2015
View reviewed changes

Xotic750 added 2 commits November 17, 2015 16:26

Merge remote-tracking branch 'upstream/master' into call

9e9c4c4

Xotic750 force-pushed the call branch from d8f337a to 9e9c4c4 Compare November 17, 2015 15:27

Xotic750 mentioned this pull request Nov 17, 2015

Patch Function#call and Function#apply together, more robust than single fix. #347

Open

Xotic750 closed this Nov 19, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `Function.prototype.call` and tests. #345

Fix `Function.prototype.call` and tests. #345

Xotic750 commented Nov 15, 2015

Xotic750 commented Nov 15, 2015

ljharb Nov 15, 2015

Xotic750 Nov 15, 2015

ljharb commented Nov 15, 2015

Xotic750 commented Nov 15, 2015

ljharb Nov 15, 2015

Xotic750 Nov 15, 2015

ljharb Nov 17, 2015

Xotic750 Nov 17, 2015

Xotic750 Nov 17, 2015

Xotic750 Nov 17, 2015

Xotic750 commented Nov 17, 2015

Xotic750 commented Nov 17, 2015

Xotic750 commented Nov 19, 2015

Fix Function.prototype.call and tests. #345

Fix Function.prototype.call and tests. #345

Conversation

Xotic750 commented Nov 15, 2015

Xotic750 commented Nov 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb commented Nov 15, 2015

Xotic750 commented Nov 15, 2015

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

Choose a reason for hiding this comment

Xotic750 commented Nov 17, 2015

Xotic750 commented Nov 17, 2015

Xotic750 commented Nov 19, 2015

Fix `Function.prototype.call` and tests. #345

Fix `Function.prototype.call` and tests. #345