Make webdriver a private dependency

[tyranron]

I've bumped crate version to 0.18.0, as webdriver upgraded from 0.43 to 0.44, so it's a breaking change.

Also, I've used 0.16.0-rc.1 for cookie as it uses 0.3 version of time. This doesn't count in public APIs, so transitioning to 0.16 should be non-breaking in future.

[tyranron]

The only failing test seems to be a flaky one.

[jonhoo]

Thanks for the PR!

Is there something in particular you need from the updated webdriver? I'd rather avoid a breaking release until we have a good reason to do one.

[tyranron]

@jonhoo not really, just outdated crates give some nerves.

I'd rather avoid a breaking release until we have a good reason to do one.

Why? The only thing a library user will need to do is to update version in Cargo.toml along with the webdriver one if it uses it directly.

I can also add changelog, describing it clearly.

Another option will be to re-export webdriver in fantoccini root, so the library users won't need to poke with the one separately, and just already compatible one:

use fantoccini::webdriver;

[jonhoo]

The big reason not to do a breaking release, even if it requires basically no action on the part of consumers, is that it still requires them to realize that the major version has changed and go update their Cargo.toml. If they don't, they'll continue being on an increasingly outdated version. Whereas if we stay on the same major version, they don't need to take any action to keep getting updates.

I'm actually not so much worried about dependents having taken dependencies on webdriver directly — I'd honestly be surprised if they have. What I'd really like to see is for us to change fantoccini so that it doesn't expose any types from webdriver (we really want it to be a private dependency). That way we'd be able to bump webdriver in the future without doing breaking releases. Now that's a change I'd be willing to cut a breaking release for :p

Any chance you'd be willing to take on the work to get rid of webdriver types that appear in the public API?

[tyranron]

@jonhoo well, I've ripped out webdriver types usages in public APIs, there weren't much:

• webdriver::common::WebWindow -> WindowHandle opaque tuple struct
• webdriver::response::NewWindowResponse -> NewWindowResponse struct + NewWindowType enum
• webdriver::capabilities::Capabilities -> Capabilities alias (actually, the same thing, but without referring upstream)

That was an "easy-peasy" part.

The harder part, where I need your guidance is whether we should to rip out webdriver::error::WebDriverError from our error::* types?
It's quite an amount of effort not only for implementing, but also for supporting that in future (a lot of duplication). But without doing it, could we really say that we won't introduce BC break on webdriver's version bump? Seems that for Displaying it's OK, but if someone has explicitly matched over WebDriverError that won't work.

[jonhoo]

Hmm, for errors I'm tempted to type-erase them to Box<dyn Error>, and then users can downcast if they truly need to get at the underlying WebDriver error. The downside of that of course is if matching on the exact error variant is common, but I don't have a great sense for how likely that is 🤔 What do you think?

[tyranron]

@jonhoo I don't think the matching on webdriver errors is common. We use fantoccini quite long and extensively in our E2E tests for doing WebRTC stuff. And we never had a case to match over errors. Just printing them was always the case.

I'm tempted to type-erase them to Box<dyn Error>, and then users can downcast if they truly need to get at the underlying WebDriver error.

I somewhat doubt that solves the problem. From some formal point of view - yes, definitely. But for the actual code that may match on webdriver::Error we don't do any better. The situation is the same, the implementor still will do need to manage its direct webdriver dependency. The only thing that has changed - we're breaking his code somewhat more silently 🙈 (instead of compilation error he will received a branch misdirection in runtime).

So, as I see, either we leave webdriver in errors and don't bother about that. Or we erase them completely without an ability to downcast into the original one.

[jonhoo]

That's true, but at least it strongly discourages matching on webdriver::Error. Your point is a good one though, it's not a great solution. How about this (painful) solution: we make our own enum error::WebDriver type that we a) mark as #[non_exhaustive] and b) manually add all the variants that exist in webdriver::WebDriver to. We also add a private helper method to convert any webdriver::Error into error::WebDriver. Then, we only use the latter type in our APIs. It's more maintenance work, but it should disconnect us from webdriver versioning.

[tyranron]

@jonhoo finally, found some time to finish this.

The "error ripping" stuff was easier than I though it would be. So now we have error::WebDriver which does't use upstream WebDriverError in public APIs, while allows library users to match over error codes by their spec values (represented as &'static str) in a fully backwards-compatible way.

Also, ripped out any webdriver type usages in From conversions, and replaced them with custom pub(crate) functions, so the webdriver types won't appear in traits too.

With all this we can say now that fantoccini public API is fully indepedent on webdriver crate. 🎉

Addiionally:

• Moved new definitions (along with the Locator) to wd module (to not be confused with webdriver crate).
• Replaced .map(|_| ()) pattern over the code with .map(drop).
• Shortened www.w3.org links to just w3.org.

The CI actually passes, the beta build failed because of:

The job exceeded the maximum log length, and has been terminated.

While stable and nightly are OK.

Final commit message for squash merge:

Remove `webdriver` crate  types from public APIs (#139)

- replace `webdriver::common::WebWindow` with `wd::WindowHandle`
- replace `webdriver::response::NewWindowResponse` -> `client::NewWindowResponse` struct + `wd::NewWindowType` enum
- replace `webdriver::capabilities::Capabilities` with `wd::Capabilities`
- replace `webdriver::error::WebDriverError` with `error::WebDriver`

Additionally:
- upgrade outdated Cargo dependencies
- enable dependabot to track outdated dependencies
- replace `.map(|_| ())` code pattern with `.map(drop)`
- shorten `www.w3.org` links to just `w3.org`

[tyranron]

ping @jonhoo

[tyranron]

ping @jonhoo

[jonhoo]

This is a very unfortunate dependency given that we expose types from cookie in the public API, and so when we switch to the non-rc version, that'll arguably be a breaking change. I may end up releasing this change as an rc as well until cookie has a full release.

[jonhoo]

Sorry it took me so long to get back to you — life got in the way :)
The pings won't generally do much. I keep an eye on my notifications, and will get to it when I can. How recently it's been touched arguably leads to lower priority since I tend to walk my notifications from oldest to newest.

Thanks (again) for all the work you're putting into this! I left more notes inline.

[tyranron]

@jonhoo

The pings won't generally do much. I keep an eye on my notifications, and will get to it when I can. How recently it's been touched arguably leads to lower priority since I tend to walk my notifications from oldest to newest.

I'm sorry, if pings were annoying. I'm just persistent at not allowing things vanishing in time 😅
Will keep in mind your workflow.

[jonhoo]

LGTM — thanks again!

[jonhoo]

Published as 0.18.0-beta.1 🎉