Fix 2 issues breaking IE8 compatibility #764

wyattoday · 2017-03-17T14:46:06Z

Fix the shim _slice to work in IE8. Also caches the "slice" shim the first time it's used.

Fix "indexof" to use the jquery ".inArray" to keep IE8 compatibility.

…first time it's used. Fix "indexof" to use the jquery ".inArray" to keep IE8 compatibility.

wyattoday · 2017-03-17T14:49:47Z

Also, this fixes issue #747

Rycochet

Mostly good, just a couple of questions on your code :-)

Rycochet · 2017-03-17T15:05:14Z

velocity.js

 		var _slice = (function() {
-			var slice = Array.prototype.slice;
+			var _fslice = Array.prototype.slice;


Don't need to change the function name

You're right, I'll change that.

Rycochet · 2017-03-17T15:05:59Z

velocity.js

-				slice = function() {
-					var i = this.length,
-							clone = [];
+				Array.prototype.slice = function(begin, end) {


Don't override global Array.slice - other things might be doing it - make sure you find places using Array.slice() and have them use this instead

If Array.slice() is failing in the first place (because it doesn't exist) I don't see the harm in making it work by defining it globally.

There's no reason for it to fail, if you look at https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Global_Objects/Array/slice#Browser_compatibility then it's pretty much always been supported - so if it's not there then assigning it is actually changing something that is out of Velocity's scope (such as replacing Array entirely) - hence safer to leave it broken and sit quietly in our own little corner and ignoring the broken site ;-)

I don't agree. Slice is supported in modern browsers, but that excludes IE8 (a relic that we still support for the next couple of years). And thus users using slice on IE8 will be using a similar shim to gain that ability on IE8. Modern browsers won't have their implementation of slice replaced at all (because the catch() block won't be called).

Still not our place to shim anything globally - if they want to shim it, then they can decide to do so, not get it as a side-effect of using an animation library.

Fair point, I'll rework that.

Rycochet · 2017-03-17T15:06:21Z

velocity.js

-							clone = [];
+				Array.prototype.slice = function(begin, end) {
+					// IE < 9 gets unhappy with an undefined end argument
+					end = (typeof end !== 'undefined') ? end : this.length;


(end !== undefined) - fully supported and slightly nicer

You're right. I'll make that change.

Rycochet · 2017-03-17T15:08:09Z

velocity.js

+					end = (typeof end !== 'undefined') ? end : this.length;
+
+					// For native Array objects, we use the native slice function
+					if (Object.prototype.toString.call(this) === '[object Array]'){


Why not just if(this.slice){ ?

You're right, that's better. I'll make that change.

Rycochet · 2017-03-17T15:10:50Z

Would be nice to get this in today too - then hopefully have a fully IE8 compatible release! :-)

…he "Array.prototype.slice" function, and instead writing to an "internal" Array.prototype._vel_slice.

Rycochet · 2017-03-17T16:27:27Z

velocity.js

+
+				// Quick exit if we've already defined the function.
+				// Just return it now rather than redefining it.
+				if (Array.prototype._vel_slice !== undefined)


This is an IIFE - called a single time and the results then stored in _slice on line 537 - it's not possible to call it again so no need to cache as it simply exists (or not)

Right, sorry, I wasn't thinking. I'll fix that.

…nce, fails once, never called again).

Rycochet · 2017-03-17T16:44:44Z

velocity.js

 		var _slice = (function() {
 			var slice = Array.prototype.slice;

 			try {
 				// Can't be used with DOM elements in IE < 9
 				slice.call(document.documentElement);
+				return Array.prototype.slice;


We're caching it above, so can save a few bytes by returning slice directly

Right, I'll fix that.

Rycochet · 2017-03-17T16:55:45Z

velocity.js

@@ -1447,7 +1488,7 @@
 				getUnit: function(str, start) {
 					var unit = (str.substr(start || 0, 5).match(/^[a-z%]+/) || [])[0] || "";

-					if (unit && CSS.Lists.units.indexOf(unit) >= 0) {
+					if (unit && $.inArray(unit, CSS.Lists.units) >= 0) {


Gah - just had a look, and $.inArray is jQuery only - we don't shim it - if you take these out I'll add a shim when I get home then do the release

Sure, I'll remove that from my branch.

… non-jQuery users.

@Rycochet

* Fix the shim _slice to work in IE8. Also caches the "slice" shim the first time it's used. Fix "indexof" to use the jquery ".inArray" to keep IE8 compatibility. * Incorporate changes suggested by @Rycochet including not overriding the "Array.prototype.slice" function, and instead writing to an "internal" Array.prototype._vel_slice. * Remove pointless check for previously defined function (it's called once, fails once, never called again). * Scratch that. Just store the actual custom function in _slice. * Return the cached slice. * Remove use of $.inArray so that @Rycochet can write a shim for it for non-jQuery users.

Fix the shim _slice to work in IE8. Also caches the "slice" shim the …

711fd41

…first time it's used. Fix "indexof" to use the jquery ".inArray" to keep IE8 compatibility.

Rycochet requested changes Mar 17, 2017

View reviewed changes

Incorporate changes suggested by @Rycochet including not overriding t…

f38738e

…he "Array.prototype.slice" function, and instead writing to an "internal" Array.prototype._vel_slice.

Rycochet reviewed Mar 17, 2017

View reviewed changes

wyattoday added 2 commits March 17, 2017 12:34

Remove pointless check for previously defined function (it's called o…

83b67a1

…nce, fails once, never called again).

Scratch that. Just store the actual custom function in _slice.

f2df168

Rycochet approved these changes Mar 17, 2017

View reviewed changes

Rycochet reviewed Mar 17, 2017

View reviewed changes

Return the cached slice.

e01a12b

Rycochet reviewed Mar 17, 2017

View reviewed changes

Remove use of $.inArray so that @Rycochet can write a shim for it for…

42be6b0

… non-jQuery users.

Rycochet merged commit 6eb4403 into julianshapiro:master Mar 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix 2 issues breaking IE8 compatibility #764

Fix 2 issues breaking IE8 compatibility #764

wyattoday commented Mar 17, 2017

wyattoday commented Mar 17, 2017

Rycochet left a comment

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet commented Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017

wyattoday Mar 17, 2017

Rycochet Mar 17, 2017 •

edited

Loading

wyattoday Mar 17, 2017

Fix 2 issues breaking IE8 compatibility #764

Fix 2 issues breaking IE8 compatibility #764

Conversation

wyattoday commented Mar 17, 2017

wyattoday commented Mar 17, 2017

Rycochet 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rycochet commented Mar 17, 2017

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

Rycochet Mar 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rycochet Mar 17, 2017 •

edited

Loading