Adding ng-jq functionality, the ability to force jqLite or any other lib... #10761

mboudreau · 2015-01-15T05:06:39Z

...rary by name to be used for angular.element

Added unit tests to check that jq function returns the right result, but couldn't test adding a fake jQuery object without failing many other tests. Maybe this is better for e2e tests? I would add some, but couldn't figure out the structure.

mboudreau · 2015-01-15T05:08:07Z

@mzgol @lgalfaso
Here's the new pull request that should fix ng-jq for angular master branch.

mgol · 2015-01-15T08:54:36Z

@mboudreau Thanks, I'll review it later today.

Note for the future - never commit anything to the master branch, this is probably the source of your problems. master should be used exclusively to pull from the upstream master so that you can easily have the same master locally as the latest master in the Angular repository.

Your changes should be made on a different branch that you can then easily rebase on top of master.

Another thing: if sth goes wrong it's not necessary to open a new PR; it's enough if you amend the commit, introduce changes, rebase on top of latest master and do "git push --force". GitHub will pick it up as long as the branch name doesn't change.

You can read this guide: http://contribute.jquery.org/commits-and-pull-requests/
This is for jQuery but things not related to commit message format are universal to working with most other open source projects, including Angular.

mboudreau · 2015-01-15T23:37:47Z

@mzgol Thanks for the tips. I already knew about them, I just didn't want to mess up things for the Angular team and I figured a redo would of been simpler. Maybe not.

Either way, it's done now :P

@petebacondarwin Is there anything else you want me to do? Like add that public api?

mgol · 2015-01-16T01:38:08Z

@mboudreau Fire grunt test in terminal, you'll see a couple of linting errors, e.g. not exporting the jq variable (it needs to be added to the beginning of the file in the /* global block.

mgol · 2015-01-16T01:40:53Z

src/Angular.js

+ * (eg. jQuery).
+ *
+ * This directive is global for the whole of the angular library, hence the directive can only
+ * be added to the top-most HTML element.


This is not true as you're catching ng-jq anywhere in the document. I'd rather copy the text from ngCsp.js:

"put the ngJq directive on the root element of the application or on the angular.js script tag, whichever appears first in the html document."

so that it's advised to put it there but not claim it won't work otherwise.

@mboudreau If you could rebase your PR over latest master after introducing remaining changes, that'd be great. It seems you branched at a commit that fails tests so it's impossible for your PR to pass tests because of that. It should be fine on latest master (am I right, @petebacondarwin?).

I would expect so @mzgol. Thanks for reviewing this one!

@mzgol @petebacondarwin I just tried merging the upstream master to my local without issue, however, the build is still failing even after an npm/bower install/update. Same error as the CI build (unless there's something different on the build server that I'm missing locally):

error: TypeError: Cannot read property '0' of undefined at _.groupBy.tap.tap.docTypes.directive (/home/michelb/Work/angular.js/docs/config/processors/pages-data.js:65:43) at Function.tap (/home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:6448:7) at lodash.ctor.(anonymous function) [as tap] (/home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:5924:31) at /home/michelb/Work/angular.js/docs/config/processors/pages-data.js:62:14 at /home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:3513:29 at forOwn (/home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:2105:15) at Function.map (/home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:3512:9) at lodash.ctor.(anonymous function) [as map] (/home/michelb/Work/angular.js/node_modules/lodash/dist/lodash.js:5924:31) at navGroupMappers.api (/home/michelb/Work/angular.js/docs/config/processors/pages-data.js:53:10) at /home/michelb/Work/angular.js/docs/config/processors/pages-data.js:192:28

Are you sure you're on a latest master? If you're on Linux/OS X, could you invoke:

git show -s --format=%ci `git merge-base HEAD master` | head

? This should show you a date from today or yesterday; if it's older, it means you didn't rebase over latest changes.

@mzgol 2015-01-21 09:53:33 +1100 is the output.

Everything looks good to me, I got all the changes. Dunno what I'm missing to build...

Apparently, Travis is having issues too, so I don't know what's going on. Weird stale files somewhere?

mgol · 2015-01-16T01:51:13Z

@petebacondarwin Is there anything else you want me to do? Like add that public api?

csp needed to be exposed to $sniffer as it's used later in the code. jq is needed only during bootstrap, the rest is left to the chosen library so I think we're good.

mgol · 2015-01-16T01:51:29Z

@mboudreau This looks mostly good, only a couple of remarks. 👍

mboudreau · 2015-01-20T00:38:53Z

@mzgol Added the fixes from your comments. Cheers.

mboudreau · 2015-01-21T00:00:29Z

@mzgol @petebacondarwin Build still failing after merge from upstream. Anyone care to help me debug? I'm trying to understand this document processor code, but there's a lot of abstraction...

gkalpak · 2015-01-21T08:03:02Z

I don't know for sure, but adding @module ng in the directive's ngdoc might help.

gkalpak · 2015-01-21T08:06:08Z

src/Angular.js

+ * @ngdoc directive
+ * @name ngJq
+ *
+ * @element html


Why specifically html ?

Should be ANY I think

mboudreau · 2015-01-22T03:39:44Z

@gkalpak Thanks for that. That indeed fixed it.

I also added all prefixes to the directive. Thanks for your direction :)

mboudreau · 2015-01-22T04:11:53Z

@mzgol CI still failing, but not because of my code:

ScriptTimeoutError: Timed out awaiting response to command "switchToFrame" after 30000 ms (WARNING: The server did not provide any stacktrace information)

Is there a way to restart the build? I'm fairly sure it's just me being unlucky :P

gkalpak · 2015-01-22T10:19:02Z

src/Angular.js

+ */
+var jq = function() {
+  if (isDefined(jq.name_)) return jq.name_;
+	var el;


Indentation should be 2 spaces (not tabs).

gkalpak · 2015-01-22T10:39:10Z

I restarted the flakey build and all is green now.
But I have left some inline comments... 😇

mgol · 2015-01-22T14:07:08Z

src/Angular.js

@@ -1457,7 +1510,9 @@ function bindJQuery() {
  }

  // bind to jQuery if present;
-  jQuery = window.jQuery;
+  var jqName = jq();
+  jQuery = jqName != null ? window[jqName] : window.jQuery;


I've missed it before: an empty ng-jq attribute should mean "use jqLite" and not "use the native detection algorithm" as it's done now. Otherwise it'll be impossible for people to opt in to jqLite if jQuery is included on the site which is one of the major points of this functionality. :)

Refs the above comment by @gkalpak.

mboudreau · 2015-01-27T21:37:24Z

@gkalpak @mzgol Build's still failing. Getting script timeouts.

mboudreau · 2015-01-30T05:08:25Z

@gkalpak @mzgol What's next gents? How do I get my little baby into Angular master?

gkalpak · 2015-01-30T08:29:19Z

src/Angular.js

+	jQuery = window.jQuery; // use default jQuery.
+	if (isDefined(jqName)) { // `ngJq` present
+		jQuery = jqName === null ? undefined : window[jqName]; // if empty; use jqLite. if not empty, use jQuery specified by `ngJq`.
+	}


Hm...I still see some tabs here 😕

@gkalpak God damnit. Fixed now.

lgalfaso · 2015-01-31T13:14:34Z

@mboudreau overall looks good, but can you please squash this into one commit?

@mzgol

…library by name to be used for angular.element fixing linting and jshint comments, as per @mzgol request fixing weird duplication of docs when merging adding other ng prefixes to be added and test to jq(), fixing documentation error by adding @module ng, changing @element to be any removing tabs, again. removing jq._name = undefined; as not useful anymore adding strict equality for tests Adding better description of ng-jq, fixing tabs/spaces changing the logic behind ngJq to be more robust; removing jQuery = window[jqName] for when jqName is empty string replacing tabs for spaces

mboudreau · 2015-01-31T14:00:28Z

@lgalfaso Done :)

lgalfaso · 2015-02-04T10:03:44Z

landed as 09ee82d

mboudreau · 2015-02-04T10:22:10Z

Woo! Thanks guys! @lgalfaso @gkalpak @mzgol @petebacondarwin

googlebot added the cla: yes label Jan 15, 2015

mboudreau mentioned this pull request Jan 15, 2015

Adding ng-jq functionality, the ability to choose your angular.element l... #10750

Closed

petebacondarwin mentioned this pull request Jan 15, 2015

enables AngularJS to use any jQuery, implementing issue #608 #2163

Closed

petebacondarwin assigned mgol Jan 15, 2015

petebacondarwin added this to the 1.4.x milestone Jan 15, 2015

petebacondarwin added branch: 1.3.x component: jqLite needs: public api needs: review severity: inconvenient type: feature and removed branch: 1.3.x labels Jan 15, 2015

mgol reviewed Jan 16, 2015
View reviewed changes

petebacondarwin mentioned this pull request Jan 20, 2015

Angular doesn't seem to work in Win8 #1853

Closed

gkalpak reviewed Jan 21, 2015
View reviewed changes

gkalpak reviewed Jan 22, 2015
View reviewed changes

mgol reviewed Jan 22, 2015
View reviewed changes

gkalpak reviewed Jan 30, 2015
View reviewed changes

mboudreau force-pushed the master branch from 89d55fd to 1de70d5 Compare January 31, 2015 13:50

mboudreau force-pushed the master branch from 1de70d5 to 0d54950 Compare January 31, 2015 13:59

gkalpak mentioned this pull request Feb 3, 2015

fix(gesture): Implement proper event dispatching when using JQuery angular/material#1367

Closed

lgalfaso closed this Feb 4, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding ng-jq functionality, the ability to force jqLite or any other lib... #10761

Adding ng-jq functionality, the ability to force jqLite or any other lib... #10761

mboudreau commented Jan 15, 2015

mboudreau commented Jan 15, 2015

mgol commented Jan 15, 2015

mboudreau commented Jan 15, 2015

mgol commented Jan 16, 2015

mgol Jan 16, 2015

petebacondarwin Jan 20, 2015

mgol Jan 20, 2015

petebacondarwin Jan 20, 2015

mboudreau Jan 20, 2015

mgol Jan 20, 2015

mboudreau Jan 20, 2015

mgol commented Jan 16, 2015

mgol commented Jan 16, 2015

mboudreau commented Jan 20, 2015

mboudreau commented Jan 21, 2015

gkalpak commented Jan 21, 2015

gkalpak Jan 21, 2015

petebacondarwin Jan 21, 2015

mboudreau commented Jan 22, 2015

mboudreau commented Jan 22, 2015

gkalpak Jan 22, 2015

gkalpak commented Jan 22, 2015

mgol Jan 22, 2015

mboudreau commented Jan 27, 2015

mboudreau commented Jan 30, 2015

gkalpak Jan 30, 2015

mboudreau Jan 31, 2015

lgalfaso commented Jan 31, 2015

mboudreau commented Jan 31, 2015

lgalfaso commented Feb 4, 2015

mboudreau commented Feb 4, 2015

Adding ng-jq functionality, the ability to force jqLite or any other lib... #10761

Adding ng-jq functionality, the ability to force jqLite or any other lib... #10761

Conversation

mboudreau commented Jan 15, 2015

mboudreau commented Jan 15, 2015

mgol commented Jan 15, 2015

mboudreau commented Jan 15, 2015

mgol commented Jan 16, 2015

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 Jan 16, 2015

mgol commented Jan 16, 2015

mboudreau commented Jan 20, 2015

mboudreau commented Jan 21, 2015

gkalpak commented Jan 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mboudreau commented Jan 22, 2015

mboudreau commented Jan 22, 2015

Choose a reason for hiding this comment

gkalpak commented Jan 22, 2015

Choose a reason for hiding this comment

mboudreau commented Jan 27, 2015

mboudreau commented Jan 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgalfaso commented Jan 31, 2015

mboudreau commented Jan 31, 2015

lgalfaso commented Feb 4, 2015

mboudreau commented Feb 4, 2015