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

WrapAll() acts like wrap() when passing a function #1843

Closed
AurelioDeRosa opened this Issue Nov 7, 2014 · 12 comments

Comments

Projects
None yet
4 participants
@AurelioDeRosa
Member

AurelioDeRosa commented Nov 7, 2014

Hi.

I've just noted that when passing a function to wrapAll(), it acts in the same way of wrap(). So, either this isn't the intended behavior (this is my take) and it should be fixed, or this is the intended behavior and a note on the relative page should be added.

This is a link to a demo that shows the issue: http://jsfiddle.net/jk6wecgz/

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 7, 2014

Member

Yes, in these cicumstances wrapAll starts acting like wrap, but this confusion only shows itself if you pass a funarg.

This shows us two paths - either we could try and squeeze wrap method by taken advantage of that similarity or we could change logic of wrapAll method for funarg argument and execute passed function only once.

We could clarify the docs also, but this is already mentioned, although indirectly.

@dmethvin?

Member

markelog commented Nov 7, 2014

Yes, in these cicumstances wrapAll starts acting like wrap, but this confusion only shows itself if you pass a funarg.

This shows us two paths - either we could try and squeeze wrap method by taken advantage of that similarity or we could change logic of wrapAll method for funarg argument and execute passed function only once.

We could clarify the docs also, but this is already mentioned, although indirectly.

@dmethvin?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 7, 2014

Member

In any case i wouldn't mind adding couple more tests for those methods

Member

markelog commented Nov 7, 2014

In any case i wouldn't mind adding couple more tests for those methods

@davidbailey00

This comment has been minimized.

Show comment
Hide comment
@davidbailey00

davidbailey00 Nov 8, 2014

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

I had a look at the code
(https://github.com/jquery/jquery/blob/master/src/wrap.js) and what's happening
is that it's looping over each element you pass in the return value of the
function, and it's then passing the index of each element as an argument to that
function, and doing this individually for each element. The docs seem to match
this behaviour in the syntax section at the top but it's not made very clear
with any examples.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJUXp0qAAoJEFzScnC7IY6DBnYP/jQxFSaETOwzIFE3kng3YqtO
ey5NIdLC5CtrkfFxy1B2sdFx+UNNRjqnVOPD4lany63P9N7iayX0r2bD6PBfME9x
Bh2khxKjdGSZTEGJdeFt0bupBvUhYclwtqOVCeQfWakpPQ3VgWeNVAr5zkIyvLDF
dB+dQW9nnthKH4ou6MnUuruvwXRgMacazIo0d1lLkxjautTdWLu7D2bSmtfW+ksJ
GUODr8egrCU4Pmwt8wXGuPXj+7QrRrxsenKsVSh324F/lgyiJBU+hOWTbueZdE1C
qqX5LCgyKk6iN6eWkb3YBBaGBHoWsGXV5BkkvEg1Uoy+KDxViRUmY+JPlB3/XZTh
NIDdhZPjdk2g5sCmQCBYpHYIMVZkJ0iAsq1UiM0Z34M/X6l8I7+cbL1lMuMsh25q
O/pqAGaN/H1zNY2vGRGxL5YFFZ9Cy6vXr+flqygWga/w5qOZDjSsMFi6ZPLQ6qhd
HmuvEs7sI7wwY8IuUbubGZXnnyKCSjfOjFf3MVohLUizZXM+kGtNxu07RnkECzxY
fkHJ727nfMBaDqCsgVM49Ha0V2frbqqOJiBOCaJqJc4HAIB8y7nNGWnSDebM9+h9
PL93xUIvV9EKokTTN+LH9afzDCnvcCWFPfhDiY+XKuKPTKejqXPstBWzWFX7d14r
f/pFnp4qoeezetqaOsmc
=p4kW
-----END PGP SIGNATURE-----

davidbailey00 commented Nov 8, 2014

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

I had a look at the code
(https://github.com/jquery/jquery/blob/master/src/wrap.js) and what's happening
is that it's looping over each element you pass in the return value of the
function, and it's then passing the index of each element as an argument to that
function, and doing this individually for each element. The docs seem to match
this behaviour in the syntax section at the top but it's not made very clear
with any examples.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCgAGBQJUXp0qAAoJEFzScnC7IY6DBnYP/jQxFSaETOwzIFE3kng3YqtO
ey5NIdLC5CtrkfFxy1B2sdFx+UNNRjqnVOPD4lany63P9N7iayX0r2bD6PBfME9x
Bh2khxKjdGSZTEGJdeFt0bupBvUhYclwtqOVCeQfWakpPQ3VgWeNVAr5zkIyvLDF
dB+dQW9nnthKH4ou6MnUuruvwXRgMacazIo0d1lLkxjautTdWLu7D2bSmtfW+ksJ
GUODr8egrCU4Pmwt8wXGuPXj+7QrRrxsenKsVSh324F/lgyiJBU+hOWTbueZdE1C
qqX5LCgyKk6iN6eWkb3YBBaGBHoWsGXV5BkkvEg1Uoy+KDxViRUmY+JPlB3/XZTh
NIDdhZPjdk2g5sCmQCBYpHYIMVZkJ0iAsq1UiM0Z34M/X6l8I7+cbL1lMuMsh25q
O/pqAGaN/H1zNY2vGRGxL5YFFZ9Cy6vXr+flqygWga/w5qOZDjSsMFi6ZPLQ6qhd
HmuvEs7sI7wwY8IuUbubGZXnnyKCSjfOjFf3MVohLUizZXM+kGtNxu07RnkECzxY
fkHJ727nfMBaDqCsgVM49Ha0V2frbqqOJiBOCaJqJc4HAIB8y7nNGWnSDebM9+h9
PL93xUIvV9EKokTTN+LH9afzDCnvcCWFPfhDiY+XKuKPTKejqXPstBWzWFX7d14r
f/pFnp4qoeezetqaOsmc
=p4kW
-----END PGP SIGNATURE-----
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 11, 2014

Member

I really dislike function arguments to methods. I'm sure it seemed like a nifty little addition to the API at the time but it just causes so much pain and bloat! @markelog and @AurelioDeRosa what do you think? It seems like enough of an edge case that we could document the current behavior if refactoring is too much of a pain. After all it's been like this for years.

Member

dmethvin commented Nov 11, 2014

I really dislike function arguments to methods. I'm sure it seemed like a nifty little addition to the API at the time but it just causes so much pain and bloat! @markelog and @AurelioDeRosa what do you think? It seems like enough of an edge case that we could document the current behavior if refactoring is too much of a pain. After all it's been like this for years.

@AurelioDeRosa

This comment has been minimized.

Show comment
Hide comment
@AurelioDeRosa

AurelioDeRosa Nov 11, 2014

Member

I think that we could start by mentioning this issue in the documentation, so that people are aware of the different behavior @dmethvin. Said that, I can see that this is will break the compatibility with some older code but, as you said, it's an edge case and I think very few developers are using the method passing a function. Based on this, if the code needed to fix the issue won't result in an increase in size of the jQuery source, we can proceed.

Member

AurelioDeRosa commented Nov 11, 2014

I think that we could start by mentioning this issue in the documentation, so that people are aware of the different behavior @dmethvin. Said that, I can see that this is will break the compatibility with some older code but, as you said, it's an edge case and I think very few developers are using the method passing a function. Based on this, if the code needed to fix the issue won't result in an increase in size of the jQuery source, we can proceed.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 11, 2014

Member

I think we could go either way, 3.0 around the corner so we could break back-compat and make these methods more consistent, it should increase the size, although not by much, or we could document it and decrease the size by exploiting this similarity

Member

markelog commented Nov 11, 2014

I think we could go either way, 3.0 around the corner so we could break back-compat and make these methods more consistent, it should increase the size, although not by much, or we could document it and decrease the size by exploiting this similarity

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 12, 2014

Member

Good point, this is a relatively small back compat break but 3.0 would be the right time to do it. So let's go ahead and call this a bug that we'll fix.

Member

dmethvin commented Nov 12, 2014

Good point, this is a relatively small back compat break but 3.0 would be the right time to do it. So let's go ahead and call this a bug that we'll fix.

@dmethvin dmethvin added this to the 3.0.0 milestone Nov 12, 2014

@markelog markelog added the Bug label Nov 12, 2014

@markelog markelog self-assigned this Nov 12, 2014

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 12, 2014

Member

okely dokely

Member

markelog commented Nov 12, 2014

okely dokely

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 16, 2014

Member

So just we're on the same page here -

this:

jQuery( "div" ).wrapAll( "<p></p>" );

would equal to:

jQuery( "div" ).wrapAll(function( /* No index argument now, since this function would be executed only once */ ) {
    // context is equal to the first found element
    return "<p></p>";
});

Cool?

Member

markelog commented Nov 16, 2014

So just we're on the same page here -

this:

jQuery( "div" ).wrapAll( "<p></p>" );

would equal to:

jQuery( "div" ).wrapAll(function( /* No index argument now, since this function would be executed only once */ ) {
    // context is equal to the first found element
    return "<p></p>";
});

Cool?

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 16, 2014

Member

Yes, I think that's the new interpretation. The function shouldn't be called at all if the set is empty I suppose?

Member

dmethvin commented Nov 16, 2014

Yes, I think that's the new interpretation. The function shouldn't be called at all if the set is empty I suppose?

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 16, 2014

Member

The function shouldn't be called at all if the set is empty I suppose?

Will add tests for it, it seems we do not have tests for funarg argument of wrapAll method at all, will add them too.

Member

markelog commented Nov 16, 2014

The function shouldn't be called at all if the set is empty I suppose?

Will add tests for it, it seems we do not have tests for funarg argument of wrapAll method at all, will add them too.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog Nov 16, 2014

Member

Will create issue for docs and migrate too.

Member

markelog commented Nov 16, 2014

Will create issue for docs and migrate too.

markelog added a commit to markelog/jquery that referenced this issue Dec 8, 2014

markelog added a commit to markelog/jquery that referenced this issue Dec 8, 2014

@markelog markelog closed this in 359b03c Dec 24, 2014

@lock lock bot locked as resolved and limited conversation to collaborators Jun 20, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.