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

CSS: Don't expose jQuery.swap #2182

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
7 participants
@mgol
Member

mgol commented Mar 30, 2015

jQuery.swap was an undocumented API used only internally. With the modular
AMD system we currently have it's not necessary to expose this function
publicly under the jQuery object.

Fixes gh-2058

CSS: Don't expose jQuery.swap
jQuery.swap was an undocumented API used only internally. With the modular
AMD system we currently have it's not necessary to expose this function
publicly under the jQuery object.

Fixes gh-2058
Closes gh-2182
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Mar 30, 2015

Member

Definitely a good idea. Now we get to find out who's using this!

Member

dmethvin commented Mar 30, 2015

Definitely a good idea. Now we get to find out who's using this!

@arthurvr

This comment has been minimized.

Show comment
Hide comment
@arthurvr

arthurvr Mar 31, 2015

Member

Sure! 👍

Member

arthurvr commented Mar 31, 2015

Sure! 👍

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 2, 2015

Member

This will also allow to change swap semantics. On master it's currently used in only one place so it might not need all its current quirks.

Member

mgol commented Apr 2, 2015

This will also allow to change swap semantics. On master it's currently used in only one place so it might not need all its current quirks.

@theodoreb

This comment has been minimized.

Show comment
Hide comment
@theodoreb

theodoreb Apr 2, 2015

It's being used in Drupal in a custom plugin we made to get the size of an horizontal menu: jquery.intrisic.js. It's used in all our admin pages for responsive menu purposes.

I haven't looked into what it'd take to replace it on our side, just noticed this issue. Totally fine with reworking the script. If you have in mind a easy way to replace this it'd be great, otherwise we'll do our homework.

It's being used in Drupal in a custom plugin we made to get the size of an horizontal menu: jquery.intrisic.js. It's used in all our admin pages for responsive menu purposes.

I haven't looked into what it'd take to replace it on our side, just noticed this issue. Totally fine with reworking the script. If you have in mind a easy way to replace this it'd be great, otherwise we'll do our homework.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 2, 2015

Member

@theodoreb thanks for the heads-up, always better to know before than after. Besides the fact that this has been undocumented, it's a real performance killer because it can cause two reflows of the affected regions. Generally it's better to do the bookkeeping of visibility yourself (e.g. via a data item or class name) so the browser won't have to do all that work.

Sounds like we may want to move the jQuery.swap() code to jQuery Migrate then as a compat warning and polyfill?

Member

dmethvin commented Apr 2, 2015

@theodoreb thanks for the heads-up, always better to know before than after. Besides the fact that this has been undocumented, it's a real performance killer because it can cause two reflows of the affected regions. Generally it's better to do the bookkeeping of visibility yourself (e.g. via a data item or class name) so the browser won't have to do all that work.

Sounds like we may want to move the jQuery.swap() code to jQuery Migrate then as a compat warning and polyfill?

@dmethvin dmethvin referenced this pull request Apr 3, 2015

Closed

Add jQuery.swap #100

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 3, 2015

Member

It might be very dangerous, without any gain, i would be more comfortable if we would "deprecate" it first by adding message in migrate but still exposing it until 4.0

Member

markelog commented Apr 3, 2015

It might be very dangerous, without any gain, i would be more comfortable if we would "deprecate" it first by adding message in migrate but still exposing it until 4.0

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 3, 2015

Member

This was never exposed. You cannot deprecate an API that was never documented - and what would be the gain? If someone didn't see it look into the docs to see there is no documentation, they wouldn't see it deprecated either. Also, it's trivial to add and will be easily handled by the Migrate plugin.

I think we should remove it.

Member

mgol commented Apr 3, 2015

This was never exposed. You cannot deprecate an API that was never documented - and what would be the gain? If someone didn't see it look into the docs to see there is no documentation, they wouldn't see it deprecated either. Also, it's trivial to add and will be easily handled by the Migrate plugin.

I think we should remove it.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Apr 3, 2015

Member

This was never exposed.

It is exposed through API. You could also see the header of this pull if you don't believe me :-)

You cannot deprecate an API that was never documented

That is why i put this word in quotes.

There is couple articles, as i recall, mentions swap, plugins that use it and real-life code depending on it.

what would be the gain

Gain would be not to break users code.

If someone didn't see it look into the docs to see there is no documentation, they wouldn't see it deprecated either

As i said above, we could add a warning to migrate first.

I think we should remove it.

I don't think we should remove it, i think we should stop exposing it, but i don't see the rush, i think there wouldn't be any point to do it right now.

Member

markelog commented Apr 3, 2015

This was never exposed.

It is exposed through API. You could also see the header of this pull if you don't believe me :-)

You cannot deprecate an API that was never documented

That is why i put this word in quotes.

There is couple articles, as i recall, mentions swap, plugins that use it and real-life code depending on it.

what would be the gain

Gain would be not to break users code.

If someone didn't see it look into the docs to see there is no documentation, they wouldn't see it deprecated either

As i said above, we could add a warning to migrate first.

I think we should remove it.

I don't think we should remove it, i think we should stop exposing it, but i don't see the rush, i think there wouldn't be any point to do it right now.

@theodoreb

This comment has been minimized.

Show comment
Hide comment
@theodoreb

theodoreb Apr 3, 2015

For the record you can take Drupal out of the real-life code depending on it. Our code can do without it, it's merely by convenience we ended up using it. The version of Drupal using swap is still in beta, so on our end there is nothing we can't break at this level.

I'd say putting it in jquery migrate is the right thing to do. Though I don't have anything in the fight since Drupal isn't and won't be using the migrate plugin.

For the record you can take Drupal out of the real-life code depending on it. Our code can do without it, it's merely by convenience we ended up using it. The version of Drupal using swap is still in beta, so on our end there is nothing we can't break at this level.

I'd say putting it in jquery migrate is the right thing to do. Though I don't have anything in the fight since Drupal isn't and won't be using the migrate plugin.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 7, 2015

Member

I've thought about this a bit more and am wondering if we are doing this backwards. Perhaps the better course would be to expose jQuery.swap and take out the "get dimensions of hidden elements" feature. That way, we let people continue to do slow things but don't hide the badness of it.

Member

dmethvin commented Apr 7, 2015

I've thought about this a bit more and am wondering if we are doing this backwards. Perhaps the better course would be to expose jQuery.swap and take out the "get dimensions of hidden elements" feature. That way, we let people continue to do slow things but don't hide the badness of it.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Apr 7, 2015

Member

@dmethvin Would it really buy those users much? I think this method looks a little arcane, it's just an utility that made sense to save space internally by not repeating logic but I think it's not really intuitive to use. If someone needs it somewhere, they need to know what they want to swap and at this point it seems easier for such a person to do this swapping logic manually.

Member

mgol commented Apr 7, 2015

@dmethvin Would it really buy those users much? I think this method looks a little arcane, it's just an utility that made sense to save space internally by not repeating logic but I think it's not really intuitive to use. If someone needs it somewhere, they need to know what they want to swap and at this point it seems easier for such a person to do this swapping logic manually.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Apr 7, 2015

Member

@mzgol agreed. This function doesn't have much utility. It makes more sense to me for the user to do it themselves.

Member

timmywil commented Apr 7, 2015

@mzgol agreed. This function doesn't have much utility. It makes more sense to me for the user to do it themselves.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Apr 8, 2015

Member

I see your point @mzgol, we're making a generic utility but someone who wanted to do this in their own code could do it with a lot less hassle. OK then let's carry on with the original plan 😺

Member

dmethvin commented Apr 8, 2015

I see your point @mzgol, we're making a generic utility but someone who wanted to do this in their own code could do it with a lot less hassle. OK then let's carry on with the original plan 😺

@timmywil timmywil added this to the 3.0.0 milestone Apr 13, 2015

@timmywil timmywil assigned timmywil and mgol and unassigned timmywil Apr 13, 2015

@mgol mgol closed this in bb4d888 Apr 13, 2015

mgol added a commit that referenced this pull request Apr 13, 2015

CSS: Don't expose jQuery.swap
jQuery.swap was an undocumented API used only internally. With the modular
AMD system we currently have it's not necessary to expose this function
publicly under the jQuery object.

Fixes gh-2058
Closes gh-2182

@mgol mgol deleted the mgol:swap branch Apr 14, 2015

mgol added a commit that referenced this pull request Nov 10, 2015

CSS: Don't expose jQuery.swap
jQuery.swap was an undocumented API used only internally. With the modular
AMD system we currently have it's not necessary to expose this function
publicly under the jQuery object.

Fixes gh-2058
Closes gh-2182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment