feat(ngRepeat): ngRepeat is now immune to removing or moving its elements without its corresponding comment node

[kamilkp]

This PR closes issues: #5054, #4954, #5619.

The ngRepeatDirective internally checks if one of its elements is being removed/inserted properly (with its corresponding comment element as ngRepeat does) or not. And if not (as sortable functions/directives do) it removes the comment node when needed, and then inserts it when needed and where it's supposed to. Now any sortable directive will work properly (e.g. https://github.com/mostr/angular-ui-multi-sortable or even raw jQuery (multi)sortable)

[tobio]

+1 Is this likely to be included in the next Angular release?

[westj]

Same here. I'm not quite sure why #5619 was closed because as far as I can tell, its definitely a bug/feature of 1.2+ which is preventing me from upgrading.

[bpampuch]

+2 ;)

[CGenie]

+1

[mgmorcos]

+1

[alboyadjian]

+1

[michaelpelletier]

+1

[caitp]

Does jQuery emit these events? I can't find any evidence of it in the 2.x.x tree, so this feels wrong to me. I think I need to investigate the issue a bit to do a proper review.

[kamilkp]

After this patch:

// in Angular.js
// Method signature:
//     jqLitePatchJQueryInsert(name)
// all jQuery methods that can insert an element into DOM
jqLitePatchJQueryInsert('append');
jqLitePatchJQueryInsert('prepend');
jqLitePatchJQueryInsert('before');
jqLitePatchJQueryInsert('after');
jqLitePatchJQueryInsert('replaceWith');
jqLitePatchJQueryInsert('wrapAll');

// in jqLite.js
function jqLitePatchJQueryInsert(name){
    var originalJqFn = jQuery.fn[name];
    originalJqFn = originalJqFn.$original || originalJqFn;
    insertPatch.$original = originalJqFn;
    jQuery.fn[name] = insertPatch;

    function insertPatch() {
      // jshint -W040
      var elementsAttached = jQuery(arguments[0]),
        originalResult = originalJqFn.apply(this, arguments);
      if(elementsAttached){
        elementsAttached.triggerHandler('$attach', elementsAttached);
      }
      return originalResult;
    }
}

It does emit these events. It is the exact same kind of jQuery function patch that alredy exists in angular.js for jQuery remove, empty and html functions. It's called jqLitePatchJQueryRemove

[mgol]

What error does this silence?

[caitp]

http://www.jshint.com/docs/options/#validthis

[mgol]

Then it's better to just do:

/* jshint validthis: true */

at the beginning of the function. jsHint comment directives are scoped, no need to rely on warning numbers that are cryptic & subject to change.

[kamilkp]

I updated the PR taking into consideration @mzgol 's remarks about jQuery append and remove signarutes

[mgol]

@kamilkp I had some other comments, at least one hasn't been taken into account yet. Just reminding. :)

[mgol]

BTW, we're monkey-patching jQuery more & more here; I feel that such modifications create the need to run the full jQuery test suite after applying those modifications to make sure nothing broke. Currently it would be extremely hard but jQuery plans to switch to Karma soon so maybe the test suite could be included.

That's a little OT for this particular pull request, though, just something to keep in mind.

[kamilkp]

@kamilkp I had some other comments, at least one hasn't been taken into account yet. Just reminding. :)

What exactly do you mean? I only didn't take into account the jshint suggestion. And that's because in angular sources silencing jshint is written the way I wrote it. I don't argue which way is better though, might as well be yours.

[kamilkp]

@caitp i just wanted to ask if it's likely that this PR will ever be merged ('couse it's been a month already) or if thats just the amount of time you guys need to review a PR?

[caitp]

I can't recall why I'm assigned to this one, but I really don't want to merge a patch that monkey-patches jQuery even more than is already happening.

[kamilkp]

I use the monkey-patch that already exists (for removes) and wrote another for inserts in the exact same way. I don't see why would it make matters with jQuery any worse (apart from the fact that everything works)

[Substr]

@kamilkp @caitp Could this issue be progressed somehow? I like many others are waiting for this to be merged and are having to use a custom branch of angular until it is. Thanks.

[kamilkp]

I would love to see it progress somehow. And the build error after my last commit in this PR is not unserstandalble to me. Can anyone point me into the right direction to help me understand what caused this error?

[mgol]

@kamilkp It seems the error is not related to your patch so amending the commit and then pushing with --force should re-run the tests.

Anyway, I agree with @caitp that it's risky to monkey-patch jQuery more. If we ever want to do it, I think it's crucial to test jQuery modified by Angular if it passes all jQuery unit tests. Until that happens (hopefully soon), I think risks outweigh the gains.

[mgol]

FYI, in a recent commit: d71dbb1 I did sth going in the opposite direction, i.e. we know no longer patch all these individual jQuery methods and instead just hook to jQuery.cleanData. For reasons of such a change read the commit message.

This PR would need to bring some of those patches back and neither me, nor @caitp, nor @IgorMinar (we talked about it recently) want to go into that direction. The risk of breaking sth is just too high, especially that we need to work with multiple jQuery versions and we currently don't even test if our patches don't break the jQuery test suite.

[IgorMinar]

+1 for what @mzgol said. we used to patch jquery because we were not aware of a better solution to ensuring proper memory cleanup. With d71dbb1 we no longer need to monkey patch many jquery methods and hope for the best.

As Tobias mentioned in #5619 (comment) having something other library modify DOM produced and controlled by Angular will always result in weird issues and can't be made work reliably.

We appreciate the PR, but it's aligned with the direction we want to go, so I'm going to close it.

[kamilkp]

I have written a module to obtain a sortable behaviour with no jQuery nor jQueryUI needed whatsoever. And it also doesn't break the ngRepeat inner logic which was the reason for submitting this PR. The module can be found here and a demo here

[mgol]

Note that jQuery UI has very good accessibility support so it's not that
it's easy to quickly replace all it does under the hood; a lot of effort
went into that. Unfortunately it's not compatible with the Angular
paradigm, though so replacement is needed for Angular projects.

[kamilkp]

I am aware of that.
My sortable module is still in very early stage, and i plan to add all of the options that jQuery sortable provides. So far i only support setting containment and handles but i will also add custom helpers and other features. All in a handy declarative way, the way that angular is designed to be used.

[mgol]

Sure, I didn't want to discourage you, I just wanted to warn people reading
it that it's far from being a complete replacement. :)

Michał Gołębiowski