CSS: Avoid filling jQuery.cssProps #4005
Conversation
|
What's the size difference? |
| @@ -0,0 +1,38 @@ | |||
| define( [ "../var/document" ], function( document ) { | |||
|
|
|||
| "use strict"; | |||
There was a problem hiding this comment.
Please add an empty line below this one; I think that's what we do elsewhere.
src/effects/Tween.js
Outdated
| @@ -14,6 +16,8 @@ Tween.prototype = { | |||
| constructor: Tween, | |||
| init: function( elem, options, prop, end, easing, unit ) { | |||
| this.elem = elem; | |||
| this.styleElem = elem.nodeType === 1 && ( jQuery.cssHooks[ prop ] || | |||
There was a problem hiding this comment.
What's the reason for this change? We're now unit-testing jQuery.Tween, in particular the behavior of its properties so it's kind of a public API now. I'd rather not expose internal properties on Tween objects.
There was a problem hiding this comment.
Mainly I was trying to avoid doing this calculation each time and just do it once in the init. It's no problem to move it back though.
|
Unit tests are failing in Travis. |
|
Not sure why I didn't see those failures. Moving the test back out of the init fixed the issue, I guess if all props in the Tween are considered fair game at any time that's not a good optimization. I did notice that in the tween.js tests we're mocking |
Fixes jquerygh-3986 Closes jquerygh-4005 Avoids filling jQuery.cssProps by introducing a second internal prop cache. This allows jQuery Migrate to detect external usage.
|
@mgol was there something else you wanted to review before I landed this? I noticed the tag. |
|
|
||
| // Return a potentially-mapped jQuery.cssProps or vendor prefixed property | ||
| function finalPropName( name ) { | ||
| var final = jQuery.cssProps[ name ] || vendorProps[ name ]; |
There was a problem hiding this comment.
Hm.. I don't think this resolved the concern from jquery/jquery-migrate#291.
The jquery-migrate pull proposes a warning for getting (and theoretically, setting) of jQuery.cssProps itself. That may seem odd, but is actually fine, given that detecting the setting of sub-properties is hard, and, in order to set a sub-property, one has to get the cssProps object first, which will trigger the warning. All good.
However, the code here in core/css.js, still gets the jQuery.cssProps object the same way as before, so it seems that would trigger the warning?
The only thing that changed is it now gets the object once instead of twice.
There was a problem hiding this comment.
In the previous code, we set a property on jQuery.cssProps. Now we no longer do that, so Migrate can look for situations where external code sets values here.
There was a problem hiding this comment.
Yep, but the warning added in jquery/jquery-migrate#291 is not for getting/setting of properties on jQuery.cssProps (of which this commit removes 1/2 instances), it warns for the getting of the cssProps property of jQuery - which it still does.
Let's continue at jquery/jquery-migrate#291 :)
Fixes gh-3986
This avoids filling jQuery.cssProps by introducing a second internal
prop cache. It allows jQuery Migrate to detect external usage.
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com