enables AngularJS to use any jQuery, implementing issue #608

[MihaiValentin]

Enables AngularJS to use any jQuery library by using the window.angularjsUseJquery variable, that can have the following values:

• undefined / null / false - the default behavior (try to use window.jQuery, if not, use internal)
• "internal" - use the internal jQuery lite, no matter which other jQuery is present in the page
• reference to any variable containing a jQuery instance

Also updated the documentation (manual bootstrapping guide) to include details about this feature.

[mhevery]

Could you describe the reasoning why you need this feature?

Also we would need unit tests.

[MihaiValentin]

Hi,

The reason for this feature is if you have an app in which you use an older version of jQuery (say 1.4.4) and Angular automatically loads it, even if methods like "prop" are not present in that jQuery version, causing errors (as Angular requires them).

In this way, we can explicitly tell Angular which jQuery to use, but if not specified, its behavior will not be changed.

Due to the fact bindJquery() is executed at the beginning of testabilityPatch.js, I don't think we can write an unit test for this. Do you have any suggestions to how testing this should be achieved?

Thanks,
Mihai.

[jbdeboer]

Just a drive-by comment if you are still working on this PR:

• you could create a set of tests that don't use testabilityPatch,
• you could make change testabilityPatch.js to call bindJquery later in the process so you have a chance to configure jquery.

[petebacondarwin]

@vmihai - are you still interested in moving this forward?

[MihaiValentin]

@petebacondarwin - I am interested, however I did not have time to set-up testing as suggested by jbdeboer.

[petebacondarwin]

@vmihai - are you likely to have time in the next few weeks? If not, I suggest we close this PR for now and when you have time you can reopen it or recreate a new one...

[biiiipy]

This is quite important in a scenario where AngularJS ir just a part in a legacy app, that uses old jQuery version (can't update), which does not work well with AngularJS.

[MihaiValentin]

I've also added an unit test. Besides jquery and jqlite, I've also added forcejqlite that sets the flag to force using jqLite and then executes all the tests.

To test this, run grunt forcejqlite.

I have also submitted the CLA form as an individual.

Thanks,
Mihai.

[MihaiValentin]

Any feedback/decision on this pull request (code or tests)?

Thanks,
Mihai.

[petebacondarwin]

I don't really like the global flag approach. I wonder if we could add a third parameter to angular.bootstrap?

angular.bootstrap(element, moduleArray, jQueryImplementation);

You then pass in either your own version of jQuery to use:

• object - use the given object as jQuery
• null - use jqLite
• undefined - use jQuery if available and jqLite otherwise (default)

@vmihai - what do you think?

[MihaiValentin]

Hi Pete,

Yes, the bootstrap idea seems solid, however bindJQuery is performed very early in the process, so at the time of the bootstrap, jQuery is already binded. Of course, we could try a rebind, but I have no idea of the impact.

Also, if we do this, apps that use the default jQuery (null to use jqLite) can only be started through manual bootstrapping, and may lead to errors if there's already a ng-app that automatically initializes it with the in-page jQuery.

Let me know what you think about this.

[biiiipy]

Global variable is not a cool solution, but it's not that bad. And maybe for greater flexibility, we should use a single global object with all (future) relevant flags? Like:

window.angularFlags = {
     jQueryVersion : null,
     ....
}

[MihaiValentin]

Yes, this sounds better and also adds support for future.

@mhevery, @petebacondarwin What do you think about this? If it's OK, I can implement it like this.

Thanks,
Mihai.

[petebacondarwin]

@vmihai

• if you are going to run multiple apps on a single page then you need to use the manual bootstrap method anyway.
• there is no reason why bindJQuery needs to be run before bootstrap, other than to search for an ng-app attribute, so we ought to be able to rework this.
• yes, this method would mean that if you are going to do something funky, like changing which jQuery is being used, you must do it explicitly by bootstrapping manually. Using ng-app and some global variable that could be lost in your code somewhere looks dangerous to me for maintaining the app in the long run. Using manual bootstrap makes it really obvious to any new developer looking at the code exactly what you are doing.

[IgorMinar]

The global variable approach causes too many issues.

I like the bootstrap option suggestion. For greater flexibility the api should be:

angular.bootstrap(element, [modules], [options])

the options map would for now contain only a single recognized property: jquery which should map to the jquery instance to use or null/undefined if internal jqlite fallback should be used even if jquery is available.

so you would use it as:

// use jquery
angular.bootstrap(element, 'myApp', { jquery: $ });

// use custom jquery
angular.bootstrap(element, 'myApp', { jquery: jquery182 });

// use jqlite even if jquery is exposed at $
angular.bootstrap(element, 'myApp', { jquery: null });

// the default
angular.bootstrap(element, 'myApp', { jquery: window.$ || null });

[petebacondarwin]

@IgorMinar - I think the default should be { jquery: undefined } meaning to use jquery if defined or jqlite otherwise. Since this is what would be the case if you didn't provide any options.

[IgorMinar]

@petebacondarwin but that's a breaking change. is that intentional?

[petebacondarwin]

What I meant was that undefined should map to the current default
behaviour

Pete
...from my mobile.
On 13 Jul 2013 18:20, "Igor Minar" notifications@github.com wrote:

@petebacondarwin https://github.com/petebacondarwin but that's a
breaking change. is that intentional?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/2163#issuecomment-20923146
.

[marcog83]

+1

[mhevery]

Closing due to inactivity. Please reopen once the issues brought up by Igor are resolved.

[marknadig]

oy, just ran into this today.I have a legacy app that is stuck on jquery 1.6. I have started to move parts over to angular and am blocked from using 1.2 rc due to jquery dependency. It would be very nice to be able to just config the angular part to use jqlite. I can see this in many scenarios w/ developers mixing in angular functionality into apps that have older jquery for various reasons.

Depending on a globally loaded library implicitly doesn't seem very angular-ish. Providing a config mechanism does.

So, seems where this was left was "what should the default be?". I would suggest making options an optional hash and it not provided or no 'jquery' value specified, just revert to the current behavior. So, no apps would break.

// existing behavior
angular.bootstrap(element, 'myApp')

// Then all of the variations that @IgorMinar mentioned above.
angular.bootstrap(element, 'myApp', {...})

[marknadig]

@petebacondarwin I'm looking at the code this morning, trying to see how this might work. You had mentioned "there is no reason why bindJQuery needs to be run before bootstrap, other than to search for an ng-app attribute, so we ought to be able to rework this."

I would love your thoughts on what this looks like; not calling bindJQuery always on load.

@IgorMinar re: not driving this with a global - seems we already have a global dependency with NG_DEFER_BOOTSTRAP. So, a pragmatic approach might be to drive this off of NG_USEJQLITE in window.name. Do 2 wrongs make a right? :)

More thoughts on this... I don't think it needs to be as complicated as provide which version of jquery we want angular to use. I believe jquery recommends only one version per page, so the angular config would seem best to be just an override to ignore window.jquery and use jqlite; ala NG_USEJGLITE

[0x-r4bbit]

+1 for @marknadig thoughts

[romanesko]

+1 for @marknadig thoughts

[MihaiValentin]

Unfortunately this solution (jQuery.fn.on) is not enough, since AngularJS still changes the global jQuery object.

This monkey patching causes conflicts with other libraries that patch jQuery, such as jQueryUI.

AngularJS should not modify global objects (like jQuery), however, even if it does, it should be an explicit option to just use the internal jqLite, even if the external one supports .on method, so they won't interfere.

[mgol]

This monkey patching causes conflicts with other libraries that patch jQuery, such as jQueryUI.

Could you provide an example? I've fixed some of those issues in Angular 1.3 and using it together with jQuery UI should work now.

[mboudreau]

What's taking so long with this? We have a breaking change because our version of jQuery is old (which we can't update throughout the system) and new development using 1.3 is bootstrapping too early, before our modules are loaded. This is not the case if jQuery is removed.

[mgol]

@mboudreau

What's taking so long with this?

Mainly the relevant lack of my free time in November. I'm commited to make it happen in Angular 1.3, I just cannot promise I'll be able to finish it up before December.

The planned solution is to have an ng-jq attribute that would works similar to how ng-csp does - we'll detect if it's on the page and if it is then:

1. if it has a value then this value is treated as a global variable that will be treated as the jQuery instance to be used by Angular
2. if the value is empty or missing, it'll fall back to jqLite.

We'll also add a separate option to angular.bootstrap for that.

[mboudreau]

Anything I can do to help? Sounds like an easy fix and could donate my time
to do it.

On Fri, Nov 21, 2014, 11:57 PM Michał Gołębiowski notifications@github.com
wrote:

@mboudreau https://github.com/mboudreau

What's taking so long with this?

Mainly the relevant lack of my free time in November. I'm commited to make
it happen in Angular 1.3, I just cannot promise I'll be able to finish it
up before December.

The planned solution is to have an ng-jq attribute that would works
similar to how ng-csp does - we'll detect if it's on the page and if it
is then:

1. if it has a value then this value is treated as a global variable
   that will be treated as the jQuery instance to be used by Angular
2. if the value is empty or missing, it'll fall back to jqLite.

We'll also add a separate option to angular.bootstrap for that.

—
Reply to this email directly or view it on GitHub
#2163 (comment).

[mgol]

If you want, you may look at how ng-csp is implemented, the mechanism for
looking for the attribute and a parameter for angular.bootstrap should be
similar to the one planned here.

If you submit a PR, please cc me on it, I'll make sure it's reviewed &
merged. Thanks!

Michał Gołębiowski

[mboudreau]

Great! Thanks

On Sat, Nov 22, 2014, 1:19 PM Michał Gołębiowski notifications@github.com
wrote:

If you want, you may look at how ng-csp is implemented, the mechanism for
looking for the attribute and a parameter for angular.bootstrap should be
similar to the one planned here.

If you submit a PR, please cc me on it, I'll make sure it's reviewed &
merged. Thanks!

Michał Gołębiowski

—
Reply to this email directly or view it on GitHub
#2163 (comment).

[mboudreau]

Alright, I started this and got a directive going, but I'm not sure where should I set the variable. The $sniffer service doesn't seem appropriate. Plus, I'm curious as to how it's going to work with the angular.bootstrap function. Do we need a 3rd parameter on the function for 'config', which could take in an object with keys like ngCsp and ngJq? Should this config be a private service itself?

[mboudreau]

@mzgol Can you please provide some direction? Thanks.

[mgol]

I'll look into it tomorrow.

Michał Gołębiowski

[mgol]

@mboudreau Sorry for the delay. After looking at what we have currently I think being able to pass an additional parameter to angular.bootstrap is not necessary. The only setting that can be passed is currently strictDi; if you don't use angular.bootstrap but the ng-app attribute instead then the strict-di attribute needs to be present on the same element ng-app is set on:

angular.js/src/Angular.js

Line 1288 in 7fd2dc1

config.strictDi = getNgAttribute(appElement, "strict-di") !== null;

This is fine since strict-di can be enabled for one Angular app and disabled for another.

Meanwhile, in the ng-csp case the only thing that is checked is the attribute being present anywhere (it doesn't have to be the root app element):

angular.js/src/Angular.js

Lines 872 to 873 in 7fd2dc1

	var active = !!(document.querySelector('[ng-csp]') ||
	document.querySelector('[data-ng-csp]'));

Since ng-jq will effectively set angular.element, it's a global and you cannot have one Angular app with jqLite and another with jQuery on the same page. Hence, we should follow the ng-csp case and just look for the attribute anywhere and not modify angular.bootstrap at all.

As for where to put it, ng-jq shouldn't be implemented as a full directive; see e.g. the rationale for why ngCsp is not a directive anymore, this is similar here:

angular.js/src/ng/directive/ngCsp.js

Lines 172 to 174 in 7fd2dc1

	// ngCsp is not implemented as a proper directive any more, because we need it be processed while we
	// bootstrap the system (before $parse is instantiated), for this reason we just have
	// the csp.isActive() fn that looks for ng-csp attribute anywhere in the current doc

Since we need the value of ng-jq only during bootstrap, we don't have to put anything in $sniffer. In the ng-csp case it's necessary as it's read later as well:

angular.js/src/ng/parse.js

Line 891 in 7fd2dc1

if (options.csp) {

The logic for finding the ng-jq attribute, reading & interpreting its value can be contained in the bindJQuery function:

angular.js/src/Angular.js

Lines 1449 to 1498 in 7fd2dc1

	function bindJQuery() {
	var originalCleanData;
	if (bindJQueryFired) {
	return;
	}
	// bind to jQuery if present;
	jQuery = window.jQuery;
	// Use jQuery if it exists with proper functionality, otherwise default to us.
	// Angular 1.2+ requires jQuery 1.7+ for on()/off() support.
	// Angular 1.3+ technically requires at least jQuery 2.1+ but it may work with older
	// versions. It will not work for sure with jQuery <1.7, though.
	if (jQuery && jQuery.fn.on) {
	jqLite = jQuery;
	extend(jQuery.fn, {
	scope: JQLitePrototype.scope,
	isolateScope: JQLitePrototype.isolateScope,
	controller: JQLitePrototype.controller,
	injector: JQLitePrototype.injector,
	inheritedData: JQLitePrototype.inheritedData
	});
	// All nodes removed from the DOM via various jQuery APIs like .remove()
	// are passed through jQuery.cleanData. Monkey-patch this method to fire
	// the $destroy event on all removed nodes.
	originalCleanData = jQuery.cleanData;
	jQuery.cleanData = function(elems) {
	var events;
	if (!skipDestroyOnNextJQueryCleanData) {
	for (var i = 0, elem; (elem = elems[i]) != null; i++) {
	events = jQuery._data(elem, "events");
	if (events && events.$destroy) {
	jQuery(elem).triggerHandler('$destroy');
	}
	}
	} else {
	skipDestroyOnNextJQueryCleanData = false;
	}
	originalCleanData(elems);
	};
	} else {
	jqLite = JQLite;
	}
	angular.element = jqLite;
	// Prevent double-proxying.
	bindJQueryFired = true;
	}

Let me know if you have further questions.

[mboudreau]

Perfect. I'll get right on that.

On Wed, Dec 10, 2014, 12:06 PM Michał Gołębiowski notifications@github.com
wrote:

@mboudreau https://github.com/mboudreau Sorry for the delay. After
looking at what we have currently I think being able to pass an additional
parameter to angular.bootstrap is not necessary. The only setting that
can be passed is currently strictDi; if you don't use angular.bootstrap
but the ng-app attribute instead then the strict-di attribute needs to be
present on the same element ng-app is set on:

angular.js/src/Angular.js

Line 1288 in 7fd2dc1

config.strictDi = getNgAttribute(appElement, "strict-di") !== null;

This is fine since strict-di can be enabled for one Angular app and
disabled for another.

Meanwhile, in the ng-csp case the only thing that is checked is the
attribute being present anywhere (it doesn't have to be the root app
element):

angular.js/src/Angular.js

Lines 872 to 873 in 7fd2dc1

	var active = !!(document.querySelector('[ng-csp]') ||
	document.querySelector('[data-ng-csp]'));

Since ng-jq will effectively set angular.element, it's a global and you
cannot have one Angular app with jqLite and another with jQuery on the same
page. Hence, we should follow the ng-csp case and just look for the
attribute anywhere and not modify angular.bootstrap at all.

As for where to put it, ng-jq shouldn't be implemented as a full
directive; see e.g. the rationale for why ngCsp is not a directive
anymore, this is similar here:

angular.js/src/ng/directive/ngCsp.js

Lines 172 to 174 in 7fd2dc1

	// ngCsp is not implemented as a proper directive any more, because we need it be processed while we
	// bootstrap the system (before $parse is instantiated), for this reason we just have
	// the csp.isActive() fn that looks for ng-csp attribute anywhere in the current doc

Since we need the value of ng-jq only during bootstrap, we don't have to
put anything in $sniffer. In the ng-csp case it's necessary as it's read
later as well:

angular.js/src/ng/parse.js

Line 891 in 7fd2dc1

if (options.csp) {

The logic for finding the ng-jq attribute, reading & interpreting its
value can be contained in the bindJQuery function:

angular.js/src/Angular.js

Lines 1449 to 1498 in 7fd2dc1

	function bindJQuery() {
	var originalCleanData;
	if (bindJQueryFired) {
	return;
	}
	// bind to jQuery if present;
	jQuery = window.jQuery;
	// Use jQuery if it exists with proper functionality, otherwise default to us.
	// Angular 1.2+ requires jQuery 1.7+ for on()/off() support.
	// Angular 1.3+ technically requires at least jQuery 2.1+ but it may work with older
	// versions. It will not work for sure with jQuery <1.7, though.
	if (jQuery && jQuery.fn.on) {
	jqLite = jQuery;
	extend(jQuery.fn, {
	scope: JQLitePrototype.scope,
	isolateScope: JQLitePrototype.isolateScope,
	controller: JQLitePrototype.controller,
	injector: JQLitePrototype.injector,
	inheritedData: JQLitePrototype.inheritedData
	});
	// All nodes removed from the DOM via various jQuery APIs like .remove()
	// are passed through jQuery.cleanData. Monkey-patch this method to fire
	// the $destroy event on all removed nodes.
	originalCleanData = jQuery.cleanData;
	jQuery.cleanData = function(elems) {
	var events;
	if (!skipDestroyOnNextJQueryCleanData) {
	for (var i = 0, elem; (elem = elems[i]) != null; i++) {
	events = jQuery._data(elem, "events");
	if (events && events.$destroy) {
	jQuery(elem).triggerHandler('$destroy');
	}
	}
	} else {
	skipDestroyOnNextJQueryCleanData = false;
	}
	originalCleanData(elems);
	};
	} else {
	jqLite = JQLite;
	}
	angular.element = jqLite;
	// Prevent double-proxying.
	bindJQueryFired = true;
	}

Let me know if you have further questions.

—
Reply to this email directly or view it on GitHub
#2163 (comment).

[antoinepairet]

Thanks for working on this. I am very interested in this feature and I would be glad to help. Ping me if I can do anything. @mboudreau where can I follow your progress on this?

[mboudreau]

I should have something done in the next 24 hours. Will include the pull
request here.

On Wed, Dec 10, 2014, 6:39 PM Antoine Pairet notifications@github.com
wrote:

Thanks for working on this. I am very interested in this feature and I
would be glad to help. Ping me if I can do anything. @mboudreau
https://github.com/mboudreau where can I follow your progress on this?

—
Reply to this email directly or view it on GitHub
#2163 (comment).

[mboudreau]

Alright, I've created the first draft and tested it thoroughly using unit tests on the jq() function and manual testing for the ng-jq attribute. It works quite well. The only issue is that I'm not sure how to test the ng-jq attribute. I should probably use e2e testing I believe, but need some direction as to where to put it. Tried to find the e2e tests for csp without any luck since they're very similar. Any further help would be appreciated.

#10430

[mboudreau]

Recreated the pull request to merge into master after some guidance from @mzgol.

#10750

[petebacondarwin]

Closing in favour of #10761