Skip to content
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

Don't fill jQuery.cssProps if mapping isn't needed #3986

Closed
dmethvin opened this Issue Feb 23, 2018 · 2 comments

Comments

Projects
None yet
2 participants
@dmethvin
Copy link
Member

commented Feb 23, 2018

While working on jquery/jquery-migrate#291 the tests failed indicating that we are using jQuery.cssProps still in a somewhat insidious way. Each time someone accesses a CSS property we're populating it with properties like { width: "width" }. That makes it hard for Migrate to warn since just about anything can end up there through no fault of external code. For now I won't land this warning and leave jquery/jquery-migrate#283 open until we can change the logic to only populate jQuery.cssProps when it needs to actually change the property name.

@dmethvin

This comment has been minimized.

Copy link
Member Author

commented Feb 23, 2018

Fixing this is messier than I'd like because we have a unit test making some pretty crazy assertions. I'm not certain that aliasing one standard CSS property to another was the intended use case but that's what we're up against. Just a thought for 4.0.0, do we even need the mapping to vendor-specific properties at that point?

@mgol

This comment has been minimized.

Copy link
Member

commented Feb 23, 2018

@dmethvin The unit test is just checking the mapping, I wouldn't expect anyone in real life aliasing top to left.

We need the mapping to cache vendor prefixed values which are still used by browsers, although not really for new APIs. There are lots of prefixed properties in IE 9+ and I think it's useful we handle it for users.

What we could do is to introduce another internal object that would be consulted if there is no match in jQuery.cssProps; we'd write vendor-prop resolved values to this separate object. This would influence the size slightly negatively but hopefully not too much?

Another solution: IIRC Migrate used to differentiate between user calls & jQuery internal ones by setting a certain flag at the beginning of a method that uses the deprecated thing and resetting it at the end (I don't remember what it was). Wouldn't something like that work here?

dmethvin added a commit to dmethvin/jquery that referenced this issue Feb 24, 2018

CSS: Avoid filling jQuery.cssProps
Fixes jquerygh-3986

This avoids filling jQuery.cssProps in the "normal" situations and
just fills it for exceptions. By doing it this way, it is posible
for jQuery Migrate to detect external usage.

@dmethvin dmethvin self-assigned this Feb 26, 2018

dmethvin added a commit to dmethvin/jquery that referenced this issue Mar 8, 2018

CSS: Avoid filling jQuery.cssProps
Fixes jquerygh-3986

This avoids filling jQuery.cssProps by introducing a second internal
prop cache. This allows jQuery Migrate to detect external usage.

@dmethvin dmethvin referenced this issue Mar 8, 2018

Merged

CSS: Avoid filling jQuery.cssProps #4005

3 of 4 tasks complete

dmethvin added a commit to dmethvin/jquery that referenced this issue Mar 20, 2018

CSS: Avoid filling jQuery.cssProps
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.

@GulajavaMinistudio GulajavaMinistudio referenced this issue Mar 21, 2018

Merged

Update upstream #43

0 of 4 tasks complete

@lock lock bot locked as resolved and limited conversation to collaborators Sep 17, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.