feat(rootEl): auto-detect the root element better #3849

sjelin · 2016-12-20T05:33:40Z

sjelin · 2016-12-20T20:05:01Z

I might have a much better idea than this, pending talking with @juliemr later today, so don't both reviewing right now

sjelin · 2016-12-21T02:37:57Z

As per talking to @juliemr, this will will not be part of a larger change after all. Please review! 😄

juliemr · 2016-12-22T19:38:02Z

lib/clientsidescripts.js

+ *
+ * @param {string} ngRepeat The ngRepeat to test
+ * @param {string} repeater The repeater to test against
+ * @param {boolean} exact If the ngRepeat needs to be more or less exactly the


Why "more or less"? Just 'exactly'

Well, it removes track by and as and everything after | or =. Is it still exactly the repeater? I don't actually understand ng1 well enough to know which parts of the ng-repeat count.

What about If the ngRepeat expression should exactly match.... it's not 100% precise because of track by but I think it gets the idea across more clearly than "more or less"

juliemr

Please add a message to the commit description that users no longer need to specify 'useAllAngular2AppRoots' in the config.

Please add a breaking change section to the commit description explaining the way that this could potentially break stuff (I don't think this will affect very many people at all, but we should have at least some sort of guidance for them on how to debug if it does)

juliemr · 2016-12-22T19:39:18Z

lib/clientsidescripts.js

+ *   falsy, tries a variety of methods to find an injector
+ * @param {boolean=} injectorPlease Prioritize finding an injector
+ * @return {$$testability?: Testability, $injector?: Injector} Returns what
+ *   the app hooks it finds


This description seems to have gotten a bit mangled.

juliemr · 2016-12-22T19:41:18Z

lib/clientsidescripts.js

+      } else {
+        var $injector = angular.element(el).injector();
+        if ($injector) {
+          return {$injector: angular.element(el).injector()};


just return {$injector: $injector} should work here, yeah?

juliemr · 2016-12-22T19:47:21Z

lib/clientsidescripts.js

-    if (!ng12Hybrid && window.getAngularTestability) {
+    if (window.angular && !(window.angular.version &&
+          window.angular.version.major > 1)) {
+      // ng1


use /** */ for comments in here.

juliemr · 2016-12-22T19:47:46Z

lib/clientsidescripts.js

-    }
-    if (angular.getTestability) {
-      angular.getTestability(el).whenStable(callback);
+    } else if (window.angular.version == 2) {


As in other PR, let's change this to be future proof for angular versions > 2

juliemr · 2016-12-22T19:55:00Z

lib/config.ts

-   * CSS Selector for the element housing the angular app - this defaults to
-   * 'body', but is necessary if ng-app is on a descendant of <body>.
+   * A CSS selector for the DOM element you want Protractor to hook into your
+   * app at.


Maybe

"A CSS Selector for a DOM element within your Angular application. Protractor will attempt to automatically find your application, but It is necessary to set rootElement in certain cases.

In Angular 1, Protractor will use bootstrapping and then search for body or ng-app elements (details here: https://git.io/v1b2r).

In later versions, Protractor will try to hook into all angular apps on the page. Use rootElement to limit the scope of which apps Protractor waits for and searches within.

"Protractor will use bootstrapping" is a little unclear but that's definitely an improvement

sjelin · 2016-12-27T20:41:29Z

lib/clientsidescripts.js

+    return {$injector: $injector, $$testability: $$testability};
+  } else {
+    return tryEl(document.body) ||
+        trySelector('[ng-app]') || trySelector('[ng:app]') ||


@juliemr should this include ng_app too?

I don't think anyone uses that... I'd leave it out. We can always add it later if there's a clamor.

sjelin · 2016-12-27T21:04:10Z

@juliemr all comments addressed

juliemr · 2016-12-29T00:27:22Z

LGTM

This is a breaking change because it changes the default root element behavior and removes the `config.useAllAngular2AppRoots` flag. Modern angular apps now default to using all app hooks, and ng1 apps now check several places, notably the element the app bootstraps to. Closes angular#1742

sjelin added the pr: needs review label Dec 20, 2016

googlebot added the cla: yes label Dec 20, 2016

sjelin force-pushed the findTheRootCause branch from 330ec6f to a8c4961 Compare December 20, 2016 05:36

sjelin removed the pr: needs review label Dec 20, 2016

juliemr force-pushed the beta branch from ef2fe60 to e3ee71c Compare December 20, 2016 20:26

sjelin force-pushed the findTheRootCause branch from a8c4961 to 51624ad Compare December 21, 2016 02:37

sjelin added the pr: needs review label Dec 21, 2016

juliemr force-pushed the beta branch from e3ee71c to 98b7f5e Compare December 21, 2016 18:10

sjelin force-pushed the findTheRootCause branch 6 times, most recently from 731c26e to 5b40ed2 Compare December 22, 2016 00:24

sjelin assigned juliemr Dec 22, 2016

juliemr reviewed Dec 22, 2016

View reviewed changes

juliemr suggested changes Dec 22, 2016

View reviewed changes

juliemr assigned sjelin Dec 22, 2016

juliemr force-pushed the beta branch from e5c7cee to 0e0490c Compare December 22, 2016 21:24

sjelin force-pushed the findTheRootCause branch from 5b40ed2 to e5ea2a2 Compare December 27, 2016 20:36

sjelin commented Dec 27, 2016

View reviewed changes

juliemr force-pushed the beta branch from 0e0490c to 07e1915 Compare December 28, 2016 17:34

sjelin force-pushed the beta branch from 07e1915 to 3644f01 Compare December 28, 2016 19:54

sjelin force-pushed the findTheRootCause branch from e5ea2a2 to 167862a Compare December 28, 2016 19:56

juliemr added pr: LGTM and removed pr: needs review labels Dec 29, 2016

juliemr modified the milestone: 5.0 Dec 29, 2016

juliemr force-pushed the beta branch from 2a68ffe to 293ffa6 Compare December 29, 2016 19:10

sjelin force-pushed the findTheRootCause branch from 167862a to 15990ad Compare January 3, 2017 19:29

sjelin force-pushed the findTheRootCause branch from 15990ad to fc61050 Compare January 3, 2017 19:44

sjelin merged commit 9a73d41 into angular:beta Jan 3, 2017

sjelin mentioned this pull request Jan 4, 2017

feat(rootEl): ***breaking change*** auto-detect the root element better #3928

Merged

feat(rootEl): auto-detect the root element better #3849

feat(rootEl): auto-detect the root element better #3849

Uh oh!

Conversation

sjelin commented Dec 20, 2016

Uh oh!

sjelin commented Dec 20, 2016

Uh oh!

sjelin commented Dec 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

juliemr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin Dec 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin commented Dec 27, 2016

Uh oh!

juliemr commented Dec 29, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sjelin Dec 27, 2016 •

edited

Loading