Create pass-through version of protractor.browser

[sjelin]

There are two main issues with the way we handle protractor.browser (i.e. global.browser):

• We call setupGlobals_ again in browser.restart, overwriting the old reference. This could cause problems if anyone saves a reference to the old browser object. For instance, in When using ExpectedConditions after browser.restart(), getting "Driver does not have valid session ID" error #3881, a test is saving a reference to ExpectedConditions, which in turn has a reference to browser, causing a problem.
• We don't really integrate .ready deeply into protractor commands. Currently this is not a problem because .ready for three reasons:
  • .ready() is only used to make sure timeouts are set correctly, and it's not a big deal if we don't wait for it properly
  • .ready() is a webdriver promise, and is waited on by the control flow
  • .ready() is waited on in Runner.run(), so this only matters when browser.forkNewDriverInstance or browser.restart() are used

#3881 is an incorrect usage, but it was a reasonable mistake. .ready() isn't a big deal at the moment, but we're going to use it more as reduce out reliance on the webdriver control flow.

I propose we make a class ShallowBrowser, which basically implements all methods/properties something like this:

class ShallowBrowser implements iProtractorBrowser {
  //  -- declare properties for ProtractorBrowser
  ...
  //  -- end property declaration

  private browserInstance_: ProtractorBrowser;
  constructor(browserInstance: ProtractorBrowser) {
    this.browserInstance_ = browserInstance;
    // Use Object.defineProperty for properties
    Object.defineProperty(this, driver, {
      get: () => {return this.browserInstance_.driver;},
      set: (driver: WebDriver) => {this.browserInstance_.driver = driver;}
    });
    ...
  }

  // Functions check `.ready()` and then pass through
  get = function() {
    this.browserInstance_.ready().then(() => {
      return this.browserInstance_.get.apply(this, arguments);
    });
  };
  ...
}

Then in restart we would set browser_.browserInstance_, instead of overwriting browser_ and the global variables.

---

To be clear, I think this is an extremely ugly solution which I hate. If you have a better idea please let me know.

@juliemr @cnishina @mgiambalvo feedback would be appreciated

[juliemr]

Another thing we could do would be to have restart and forkNewDriverInstance return promises that resolve when it's ready, so your code with async/await would look more like:

await browser.restart();
await browser.get(...);

[sjelin]

We will have to return a promise from browser.restart either way. But how would this interact with restartBrowserBetweenTests? Jasmine doesn't provide the functionality we need to wait on the returned promise.

Also, returning a promise from forkNewDriverInstance would be pretty annoying for people currently relying on the control flow, since they can't await it (as that would break the control flow), so they'd have to then it, which would be different from all the other protractor commands (and a breaking change).

[sjelin]

That also doesn't really do anything to resolve the stale reference problem

[sjelin]

If we replace the .ready property with a waitFor() API, we could probably wrap/unwrap the functions on the protractor instance as needed.

[sjelin]

Closing in favor of #3892, #3895, #3896, and #3897. This won't solve the problem of stale references to the global browser object, but it will solve everything else.