parent is null in JQLite code

[adamreisnz]

Sentry is reporting an error in our app in Angular's source code for JQLite:

The line in question is https://github.com/angular/angular.js/blob/master/src/jqLite.js#L986

It appears under some circumstances parent can be null, in which case this error pops up. It's probably a good idea to guard against unexpected values of parent so this error doesn't occur.

This was seen happening on Angular 1.5.8, on Firefox v26 on Windows XP.

[gkalpak]

There are many circumstances under which parent can be null. The question is who/what is calling after() on an element without a parent.

That said, jQuery seems to silently ignore a missing parent, so it might be a good idea to have the same behavior in jqLite. Thoughts, @mgol?

[lgalfaso]

I am somehow torn on this issue. On one side, not checking for the parent in after is bad. On the other side, silently ignoring might cause some difficult to debug issues.

[mgol]

This is the jQuery implementation: https://github.com/jquery/jquery/blob/3.1.1/src/manipulation.js#L365-L371:

    after: function() {
        return domManip( this, arguments, function( elem ) {
            if ( this.parentNode ) {
                this.parentNode.insertBefore( elem, this.nextSibling );
            }
        } );
    },

It's specifically guarded and documented:

Prior to jQuery 1.9, .after() would attempt to add or change nodes in the current jQuery set if the first node in the set was not connected to a document, and in those cases return a new jQuery set rather than the original set. The method might or might not have returned a new result depending on the number or connectedness of its arguments! As of jQuery 1.9, .after(), .before(), and .replaceWith() always return the original unmodified set. Attempting to use these methods on a node without a parent has no effect—that is, neither the set nor the nodes it contains are changed.

jqLite should primarily align with jQuery where it doesn't introduce a lot of code so it seems a good idea to fix it.

Generally, I think a good rule if you think some behavior is undesired in jqLite and that behavior is shared with jQuery is to report an issue to the jQuery bug tracker and make the change there first.

[lgalfaso]

Agree with @mgol that we should align with jQuery. That said, when the node
has no parent, should this be logged?

On Mon, Oct 31, 2016 at 1:35 PM, Michał Gołębiowski <
[email protected]> wrote:

This is the jQuery implementation: https://github.com/jquery/
jquery/blob/3.1.1/src/manipulation.js#L365-L371:

after: function() {
    return domManip( this, arguments, function( elem ) {
        if ( this.parentNode ) {
            this.parentNode.insertBefore( elem, this.nextSibling );
        }
    } );
},

It's specifically guarded and documented:

Prior to jQuery 1.9, .after() would attempt to add or change nodes in the
current jQuery set if the first node in the set was not connected to a
document, and in those cases return a new jQuery set rather than the
original set. The method might or might not have returned a new result
depending on the number or connectedness of its arguments! As of jQuery
1.9, .after(), .before(), and .replaceWith() always return the original
unmodified set. Attempting to use these methods on a node without a parent
has no effect—that is, neither the set nor the nodes it contains are
changed.

jqLite should primarily align with jQuery where it doesn't introduce a lot
of code so it seems a good idea to fix it.

Generally, I think a good rule if you think some behavior is undesired in
jqLite and that behavior is shared with jQuery is to report an issue to the
jQuery bug tracker and make the change there first.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#15331 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAX44ukbD8cFJXPvFp1fo03Q6gSO8LTSks5q5eCogaJpZM4Kj_B_
.

[gkalpak]

If we decide to silently not-do-anything (as jQuery does), then we shouldn't log imo.

[mgol]

I agree with @gkalpak. This seems a non-breaking feature to me so we can land it whenever we want. @adambuczynski would you like to try to create a patch?

[adamreisnz]

@mgol yes sure, will do