Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

test(jQuery): test not firing $destroy on jQuery.cleanData with jQuery UI #8695

Merged
merged 1 commit into from
Mar 22, 2017
Merged

test(jQuery): test not firing $destroy on jQuery.cleanData with jQuery UI #8695

merged 1 commit into from
Mar 22, 2017

Conversation

mgol
Copy link
Member

@mgol mgol commented Aug 20, 2014

So far it wasn't tested that Angular's logic for skipping it triggering
the $destroy event on jQuery.cleanData in the replaceWith internal function
works correctly when Angular is not the last one to patch the cleanData method
(e.g. if jQuery UI does the patching later). This commits adds the relevant
test.

Ref #8486

cc @IgorMinar

@mgol
Copy link
Member Author

mgol commented Aug 20, 2014

I verified that if the patch is modified to mess with Angular by changing the value of the internal skipDestroyOnNextJQueryCleanData variable to false to force Angular to not omit the patch when it should, the added test fails. I verified that when applying this PR to commit c306bc2 from PR #8486, though but on master such a modification doesn't trigger the error. I bisected it and the first commit where this happens is b5f7970

The (expected) failure I'm talking about is as follows:

Chrome 36.0.1985 (Mac OS X 10.9.4) $compile compile phase compiler control templateUrl when directive is in a repeater should work in jqLite and jQuery with jQuery.cleanData last patched by Angular FAILED
    Expected 'i={{i}};i={{i}};' to equal 'i=1;i=2;'.
    Error: Expected 'i={{i}};i={{i}};' to equal 'i=1;i=2;'.
        at null.<anonymous> (/Users/mgol/Documents/projects/public/angular.js/test/ng/compileSpec.js:1545:38)
        at Object.invoke (/Users/mgol/Documents/projects/public/angular.js/src/auto/injector.js:801:17)
        at workFn (/Users/mgol/Documents/projects/public/angular.js/src/ngMock/angular-mocks.js:2242:20)
        at window.inject.angular.mock.inject (/Users/mgol/Documents/projects/public/angular.js/src/ngMock/angular-mocks.js:2214:42)
        at runTest (/Users/mgol/Documents/projects/public/angular.js/test/ng/compileSpec.js:1538:13)
Chrome 36.0.1985 (Mac OS X 10.9.4) $compile compile phase compiler control templateUrl when directive is in a repeater should work with another library patching jQuery.cleanData after Angular FAILED
    Expected 'i={{i}};i={{i}};' to equal 'i=1;i=2;'.
    Error: Expected 'i={{i}};i={{i}};' to equal 'i=1;i=2;'.
        at null.<anonymous> (/Users/mgol/Documents/projects/public/angular.js/test/ng/compileSpec.js:1545:38)
        at Object.invoke (/Users/mgol/Documents/projects/public/angular.js/src/auto/injector.js:801:17)
        at workFn (/Users/mgol/Documents/projects/public/angular.js/src/ngMock/angular-mocks.js:2242:20)
        at window.inject.angular.mock.inject (/Users/mgol/Documents/projects/public/angular.js/src/ngMock/angular-mocks.js:2214:42)
        at runTest (/Users/mgol/Documents/projects/public/angular.js/test/ng/compileSpec.js:1538:13)
        at null.<anonymous> (/Users/mgol/Documents/projects/public/angular.js/test/ng/compileSpec.js:1564:15)
Chrome 36.0.1985 (Mac OS X 10.9.4): Executed 4 of 3138 (2 FAILED) (skipped 3134) (0.878 secs / 0.109 secs)

Apply this PR to commit c306bc2 and add the following line:

skipDestroyOnNextJQueryCleanData = false;

the the cleanData patch in the test (the one that increments cleanedCount) to see it.

@mgol
Copy link
Member Author

mgol commented Aug 20, 2014

Anyway, this got me interested as it seems that now even if I removed this whole logic of not triggering $destroy in the replaceWith function is removed, all the tests still pass. @IgorMinar, could you see what's going on here?

@mgol mgol mentioned this pull request Aug 20, 2014
Closed
@btford btford added this to the 1.3.0 milestone Aug 20, 2014
@IgorMinar
Copy link
Contributor

I think (I haven't verified this) that the tests now pass because of a change that caused element transclusion not to register a listener on the transcluded element. The relevant change: b5f7970

I would suggest writing a test that explicitly puts data on the DOM rather than relies on a behavior of other directives which can change at any time.

@mgol
Copy link
Member Author

mgol commented Aug 23, 2014

OK, I'll look into this.

@mgol mgol self-assigned this Aug 23, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0, 1.3.0-rc.1 Aug 25, 2014
@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.1, 1.3.0-rc.2 Sep 9, 2014
@tbosch tbosch modified the milestones: 1.3.0-rc.2, 1.3.0-rc.3 Sep 15, 2014
@jeffbcross jeffbcross modified the milestones: 1.3.0-rc.3, 1.3.0 Sep 22, 2014
@jeffbcross jeffbcross force-pushed the master branch 3 times, most recently from 8292c21 to 7b9fddf Compare October 9, 2014 20:43
@mgol
Copy link
Member Author

mgol commented Oct 9, 2014

@IgorMinar I'll look into it this weekend.

@mgol mgol mentioned this pull request Jul 21, 2015
Closed
@petebacondarwin petebacondarwin modified the milestones: 1.5.x - migration-facilitation, 1.3.x - superluminal-nudge Sep 7, 2015
@petebacondarwin
Copy link
Contributor

Moving to 1.5 - @mzgol & @jbedard can we try to combine this with #12094?

@mgol
Copy link
Member Author

mgol commented Sep 7, 2015

@petebacondarwin Sure!

@Narretz
Copy link
Contributor

Narretz commented Apr 9, 2016

#12094 landed last November, how does this affect this PR?

@Narretz
Copy link
Contributor

Narretz commented Nov 5, 2016

Is this still relevant?

@mgol
Copy link
Member Author

mgol commented Mar 15, 2017

Three years later, PR rebased. :) Yes, @Narretz, it's still relevant. Fortunately, the tests still pass and I could even enable them in jqLite as well now that jqLite implements the cleanData method as well.

@mgol
test(jqLite): test not firing $destroy on jqLite.cleanData with jQuer…
8efa08a 
…y UI

So far it wasn't tested that Angular's logic for skipping it triggering
the $destroy event on jQuery.cleanData in the replaceWith internal function
works correctly when Angular is not the last one to patch the cleanData method
(e.g. if jQuery UI does the patching later). This commits adds the relevant
test.

Ref angular#8486
@mgol
Copy link
Member Author

mgol commented Mar 15, 2017

The tests fail on jqLite when I enabled those cleanData tests there and not just in jQuery (where they do pass).

I think the current implementation of jqLite where it doesn't use cleanData from the JQLite object but instead uses a cached version is incorrect and that those tests catch exactly that. I'd like to change that but I'll put that in a separate PR once this lands.

@petebacondarwin petebacondarwin merged commit bf7685a into angular:master Mar 22, 2017
@mgol mgol deleted the cleanData-test branch March 22, 2017 11:06
mgol added a commit to mgol/angular.js that referenced this pull request Mar 22, 2017
@mgol
fix(jqLite): make jqLite invoke jqLite.cleanData as a method
cdb16ba 
The previous implementation of jqLite didn't use cleanData from the jqLite
object but instead used a cached version which maede it impossible to
monkey-patch jqLite.cleanData similarly to how you can do it in jQuery.

This commit enables one of the tests so far run only with jQuery to run with
jqLite as well.

Ref angular#8486
Ref angular#8695
@mgol mgol mentioned this pull request Mar 22, 2017
Closed
3 tasks
mgol added a commit to mgol/angular.js that referenced this pull request Mar 22, 2017
@mgol
fix(jqLite): make jqLite invoke jqLite.cleanData as a method
822e745 
The previous implementation of jqLite didn't use cleanData from the jqLite
object but instead used a cached version which maede it impossible to
monkey-patch jqLite.cleanData similarly to how you can do it in jQuery.

The cleanData method is not meant to be called directly by userland code;
its purpose is mainly to be able to be monkey-patched; therefore, the previous
implementation didn't make a lot of sense.

This commit enables one of the tests so far run only with jQuery to run with
jqLite as well.

Ref angular#8486
Ref angular#8695
@mgol
Copy link
Member Author

mgol commented Mar 22, 2017

A followup (related to my last comment): #15846.

mgol added a commit that referenced this pull request Mar 27, 2017
@mgol
fix(jqLite): make jqLite invoke jqLite.cleanData as a method
bf5c2ee 
The previous implementation of jqLite didn't use cleanData from the jqLite
object but instead used a cached version which maede it impossible to
monkey-patch jqLite.cleanData similarly to how you can do it in jQuery.

The cleanData method is not meant to be called directly by userland code;
its purpose is mainly to be able to be monkey-patched; therefore, the previous
implementation didn't make a lot of sense.

This commit enables one of the tests so far run only with jQuery to run with
jqLite as well.

Ref #8486
Ref #8695
Closes #15846
mgol added a commit that referenced this pull request Mar 27, 2017
@mgol
fix(jqLite): make jqLite invoke jqLite.cleanData as a method
9cde98c 
The previous implementation of jqLite didn't use cleanData from the jqLite
object but instead used a cached version which maede it impossible to
monkey-patch jqLite.cleanData similarly to how you can do it in jQuery.

The cleanData method is not meant to be called directly by userland code;
its purpose is mainly to be able to be monkey-patched; therefore, the previous
implementation didn't make a lot of sense.

This commit enables one of the tests so far run only with jQuery to run with
jqLite as well.

(cherry-picked from bf5c2ee)

Ref #8486
Ref #8695
Closes #15846
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@mgol mgol

Labels
cla: yes type: test
Projects
None yet
Milestone
1.5.x
Development

Successfully merging this pull request may close these issues.
8 participants
@mgol @IgorMinar @petebacondarwin @Narretz @jeffbcross @btford @tbosch @mary-poppins