Enhancement: `Phantom` class to replace `run_phantomjs()` list output

[jesse-smith]

Disclaimer: I'm relatively new to R6 and PhantomJS

Would it make sense to create a small R6 subclass of process for the return value of run_phantomjs()?

As I'm envisioning it, the class (i.e. Phantom) would just additional fields for port and (possibly) host, and a session instantiation method (i.e. phtm <- Phantom$new(); sesh <- phtm$session()). Additional methods could be added to make life easier for the user, but wouldn't be required for a basic implementation.

Pros

• Reference Semantics: Since the main functionality of the package is already in the Session R6 class, it seems like it would make sense to keep those reference semantics for the PhantomJS process as well (especially since process is already an R6 class).
• Object Relations: Sessions only work when connected to a PhantomJS process. This is currently not clear from the implementation alone (although admittedly likely an easy guess for the target user of webdriver). Spawning a Session from an instantiated Phantom object makes this relationship much clearer (whether by passing the Phantom object to Session$new() or by creating a Phantom$session() method). As a bonus, the user doesn't need to extract information from the object- it just works.
• Browsing Experience: The above deals with relations between object in R. A closely related point is that these semantics also mimic what's actually happening in the background in the PhantomJS process, as well as roughly mimicking the experience of browsing with tabs. To me, this makes the user experience more transparent and conceptually cleaner.

Cons

• Breaking Change: The main issue I see is that this has to be a breaking change from the current list implementation in cases where the process or port is accessed by position (i.e. proc <- pjs[[1]]) or to return another list (port_lst <- pjs['port']). Notably, this does not break accessing port by name using $ or [[. Compatibility with accessing process using the same methods can also be maintained with a process field that returns self. I'm also not entirely sure what the use case for returning a length 1 list is here, so maybe that's less of an issue.
• Reference Semantics: It's worth noting that many R users aren't very comfortable with OOP and modify-in-place semantics, so the first Pro is also a Con. With the current list implementation, I don't ever need to know that the list contains an R6 process object; I just get the port, stick it in Session$new(), and go. Granted, I don't find this a convincing argument, since you need to use the R6 Session class to actually do anything in webdriver anyway.

One last point; if you find this convincing, it may make more sense to create a general WebDriver or Driver class, rather than a Phantom class, that could be extended for use with backends other than PhantomJS. I'm not sure what the roadmap/timeline (if any) is for those extensions, just that theoretical support is mentioned in the README, and there are a few related open issues.

[gaborcsardi]

Thanks for the suggestion! It would certainly make sense to make the phantom process more prominent, in terms of API.

However, it is unlikely that webdriver will get bigger updates like this, as phantom.js is not maintained anymore, and our plan is to transition to using https://github.com/rstudio/chromote

[jesse-smith]

Got it- thanks! chromote seems to implement a very similar idea with the Chromote and ChromoteSession objects.