feat(jQuery): upgrade to jQuery to 2.1.1 #7311

mgol · 2014-04-30T10:32:51Z

feat(jQuery): upgrade to jQuery to 2.1.1

The data jQuery method was re-implemented in 2.0 in a secure way. This made
current hacky Angular solution to move data between elements via changing the
value of the internal node[jQuery.expando] stop working. Instead, just copy the
data from the first element to the other one.

Testing cache leaks on jQuery 2.x is not possible in the same way as it's done
in jqLite or in jQuery 1.x as there is no publicly exposed data storage. One
way to test it would be to intercept all places where a jQuery object is created
to save a reference to the underlaying node but there is no single place in the
jQuery code through which all element creation passes (there are various
shortcuts for performance reasons). Instead we rely on jqLite.cache testing
to find potential data leaks.

BREAKING CHANGE: Angular no longer supports jQuery versions below 2.1.1.

Original description:
The data jQuery method was re-implemented in 2.0 in a secure way. This made
current hacky Angular solution to move data between elements via changing the
value of the internal node[jQuery.expando] stop working. Instead, just move the
data from the first element to the other one.

cc @caitp @IgorMinar

mgol · 2014-04-30T10:41:32Z

src/ng/compile.js

-        jqLite(element).remove(); // must do this way to clean up expando
-        fragment.appendChild(element);
-        delete elementsToRemove[k];
+      jqLite(newNode).data(elementsToRemove.data());


This will move only user data, not private one. Private data mostly holds event handlers; do we need to keep it here? If that's the case, I'll need to add:

jqLite(newNode)._data(elementsToRemove._data());

here as well. Note, though, that this is private interface and it's even no longer used internally in jQuery code base so there is no guarantee it'll survive in the future. It'd be better to not rely on that if possible.

One problem is ngAnimate currently uses _data().events for its DOM operations and I'm not sure how important it is to be able to use exactly this interface for that. cc @matsko

Note that this may be less performant than the previous way since all the data will be moved to the new jQuery object via extend-ing it. There is no way to switch the data between elements in jQuery 2, though (making it possible was never intentional); apart from making private data private the expando key is now not pure jQuery.expando, it has a random number appended; see:
https://github.com/jquery/jquery/blob/bbace100a3ad51287cd2864eeb03ddebb381d44f/src/data/Data.js#L17

There is not a lot of data to move; we're mostly attaching $scope, $isolateScope and a couple of other things and they're obviously moved by reference but we can potentially have a lot of directives with replace: true in the page so I wonder how much of an issue this is. Do we have any perf tests?

@matsko From what I see _data is used only to get events and only internal $animate: -prefixed events are read from that. Can it be done differently, without relying on this hack? jQuery 2.x .data() implementation will change again in an upcoming release, it'll use WeakMaps and it's possible it'll remove obsolete deprecated stuff like _data so we need to think about it now.

@matsko I looked at the code & I think current implementation is abusing events a little. The '$animate:'-prefixed events are only triggerHandler-ed anyway so you bail out on bubbling. Perhaps it would be better to define a method on the $animate service that would publish those events to which you'd subscribe.

Again, we need to think about this now, before 2.2.0 is released and things like that will be set in stone; especially that it's highly likely ._data() will go away soon.

@matsko Are '$animate:'-prefixed events documented anywhere? In the code base I see handlers for them attached only in tests and the string$animate: doesn't appear in a lot of the code base. Am I missing sth?

Hey I just say these. Do you still need my help on them?

@matsko It seems ._data() will stay there at least for some time and we'll need it in another place for compatibility with obsolete replace: true directives anyway so we're OK here for now. :)

But if you'll need to use jQuery internals in Angular some time, ping me, maybe there's a better way.

Sounds good.

mgol · 2014-04-30T22:21:24Z

It would probably be better to change the $compile behavior so that we don't need to copy data from replaced elements.

What's the expected output of things like:

<div>
    <custom-directive ng-click="doSomething()"></custom-directive>
</div>

when customDirective has replace:true?

matsko · 2014-07-14T14:30:50Z

@mzgol do you still need any input from me regarding $animate-related API stuff?

mgol · 2014-07-14T14:37:03Z

@matsko Not for now. It'd be good to return to it later, maybe sth on the jQuery side could be done to avoid having to hook to private interfaces but it doesn't block the upgrade to jQuery 2 so we can leave it for later.

matsko · 2014-07-14T14:48:40Z

@mzgol Ofcoarse. Please send me an email directly when you get around to it.

mgol · 2014-07-30T13:59:12Z

@IgorMinar The patch is ready for review.

IgorMinar · 2014-07-30T17:08:26Z

docs/content/misc/faq.ngdoc

-jQuery 1.7.1 or above. However, Angular does not currently support jQuery 2.x or above.
+jQuery 1.7.1 or above. However, Angular 1.2 does not currently support jQuery 2.x or above.
+
+Angular 1.3 supports jQuery 2.1 or above.


we should say "only supports jQuery 2.1 and above". no?

IgorMinar · 2014-07-30T17:24:45Z

otherwise this is very good! thanks @mzgol!

mgol · 2014-07-30T18:03:35Z

Review comments addressed; PR rebased over current master.

mgol · 2014-07-30T18:27:06Z

I had to change some jQuery mentions to window.jQuery because in test:modules jQuery is not exported.

~~I see the builds are in the queue: https://travis-ci.org/angular/angular.js/builds/31263921. I thought they're supposed to start immediately now for PRs?~~
EDIT: I amended the commit 4 hours later and now it works; perhaps Travis was broken for a while.

mgol · 2014-07-30T18:54:45Z

src/Angular.js

-  // Angular 1.2+ requires jQuery 1.7.1+ for on()/off() support.
-  if (jQuery && jQuery.fn.on) {
+  // Angular 1.3+ requires jQuery 2.1+.
+  if (jQuery && Number(jQuery.fn.jquery.match(/[0-9]+\.[0-9]+/)[0]) >= 2.1) {


@IgorMinar I'm starting to think I should revert this change. Testing for jQuery.fn.on makes sense since we rely on this method but I'm not convinced we should shut the door so completely against jQuery versions we don't officially support. It may make sense to just claim "you're on your own now" without blocking.

What do you think?

looking through these changes, there is a good chance that things will work ok with older jQuery, so being this strict might cause pain without good justification. I'm fine with "you are on your own" policy. that's what we've been doing for quite a while.

OK, I'll revert this line. Should I also add sth along "Angular 1.3 officially supports only jQuery 2.1.1 or newer though it might work with older ones" to the docs?

The only real hard requirement is jQuery.fn.on i.e. jQuery 1.7.

mgol · 2014-07-30T20:39:23Z

I reverted the patch to make jqLite._data(key, value) a setter; I needed it for some intermediary version of this PR but it turned out to be not needed after all.

The data jQuery method was re-implemented in 2.0 in a secure way. This made current hacky Angular solution to move data between elements via changing the value of the internal node[jQuery.expando] stop working. Instead, just copy the data from the first element to the other one. Testing cache leaks on jQuery 2.x is not possible in the same way as it's done in jqLite or in jQuery 1.x as there is no publicly exposed data storage. One way to test it would be to intercept all places where a jQuery object is created to save a reference to the underlaying node but there is no single place in the jQuery code through which all element creation passes (there are various shortcuts for performance reasons). Instead we rely on jqLite.cache testing to find potential data leaks. BREAKING CHANGE: Angular no longer supports jQuery versions below 2.1.1.

mgol · 2014-07-31T20:21:43Z

Landed!

This was referenced Apr 30, 2014

Note in the docs what version of jQuery is allowed #6436

Closed

feat(jQuery): upgrade to jQuery to 2.0 [WIP] #4271

Closed

mgol reviewed Apr 30, 2014
View reviewed changes

mgol changed the title ~~feat(jqLite): make Angular jQuery 2.x-compatible~~ WIP feat(jqLite): make Angular jQuery 2.x-compatible Apr 30, 2014

btford added type: feature and removed type: feature labels May 1, 2014

mgol added cla: yes and removed cla: no labels May 1, 2014

mgol mentioned this pull request May 11, 2014

only use window.jQuery if it supports required jQuery.fn.on #6376

Merged

Narretz added the component: jqLite label Jul 2, 2014

Narretz added this to the Backlog milestone Jul 2, 2014

mgol closed this Jul 12, 2014

mgol deleted the jquery-2-compat branch July 12, 2014 23:35

mgol restored the jquery-2-compat branch July 13, 2014 15:21

mgol reopened this Jul 13, 2014

mgol modified the milestones: 1.3.0-beta.16, Backlog Jul 13, 2014

mgol self-assigned this Jul 13, 2014

btford modified the milestones: 1.3.0-beta.17, 1.3.0-beta.16 Jul 18, 2014

jeffbcross modified the milestones: 1.3.0-beta.18, 1.3.0-beta.17 Jul 25, 2014

mgol changed the title ~~WIP feat(jqLite): make Angular jQuery 2.x-compatible~~ feat(jQuery): upgrade to jQuery to 2.1.1 Jul 30, 2014

IgorMinar reviewed Jul 30, 2014
View reviewed changes

mgol reviewed Jul 30, 2014
View reviewed changes

mgol merged commit 9e7cb3c into angular:master Jul 31, 2014

mgol deleted the jquery-2-compat branch July 31, 2014 20:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(jQuery): upgrade to jQuery to 2.1.1 #7311

feat(jQuery): upgrade to jQuery to 2.1.1 #7311

mgol commented Apr 30, 2014

mgol Apr 30, 2014

mgol Apr 30, 2014

mgol Apr 30, 2014

mgol Apr 30, 2014

mgol Apr 30, 2014

mgol May 1, 2014

matsko May 13, 2014

mgol May 15, 2014

matsko May 25, 2014

mgol commented Apr 30, 2014

matsko commented Jul 14, 2014

mgol commented Jul 14, 2014

matsko commented Jul 14, 2014

mgol commented Jul 30, 2014

IgorMinar Jul 30, 2014

mgol Jul 30, 2014

IgorMinar commented Jul 30, 2014

mgol commented Jul 30, 2014

mgol commented Jul 30, 2014

mgol Jul 30, 2014

IgorMinar Jul 31, 2014

mgol Jul 31, 2014

mgol commented Jul 30, 2014

mgol commented Jul 31, 2014

feat(jQuery): upgrade to jQuery to 2.1.1 #7311

feat(jQuery): upgrade to jQuery to 2.1.1 #7311

Conversation

mgol commented Apr 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Apr 30, 2014

matsko commented Jul 14, 2014

mgol commented Jul 14, 2014

matsko commented Jul 14, 2014

mgol commented Jul 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IgorMinar commented Jul 30, 2014

mgol commented Jul 30, 2014

mgol commented Jul 30, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgol commented Jul 30, 2014

mgol commented Jul 31, 2014