search

SeleniumHQ / selenium

A browser automation framework and ecosystem.
https://selenium.dev
Apache License 2.0
29.88k stars 8.04k forks source link

[🚀 Feature]: remove guava from client bindings #12737

Open joerg1985 opened 10 months ago

joerg1985 commented 10 months ago

Feature and motivation

When using Java 11 we should not need the guava library in the client bindings not realy, we can do some small changes to remove them. Removing them from the dependencies should be done with Selenium 5, as client code could relay on it on the class path. We might be able to remove it from the server too, but this must be evaluated and could be done later.

Usage example

Less dependencies are in general better.

github-actions[bot] commented 10 months ago

@joerg1985, thank you for creating this issue. We will troubleshoot it as soon as we can.

Info for maintainers

Triage this issue by using labels.

If information is missing, add a helpful comment and then I-issue-template label.

If the issue is a question, add the I-question label.

If the issue is valid but there is no time to troubleshoot it, consider adding the help wanted label.

If the issue requires changes or fixes from an external project (e.g., ChromeDriver, GeckoDriver, MSEdgeDriver, W3C), add the applicable G-* label, and it will provide the correct link and auto-close the issue.

After troubleshooting the issue, please add the R-awaiting answer label.

Thank you!

titusfortner commented 10 months ago

Why would we wait for Selenium 5? Once we require Java 11, if we can update syntax and methods and dependencies we should just do it.

diemol commented 10 months ago

I understand the reasoning, a user might be relying on Selenium providing Guava as a dependency. But I think we can tell them what to do in the release notes.

joerg1985 commented 10 months ago

Okay, this will be a breaking change: https://github.com/SeleniumHQ/selenium/blob/e5182732e95bddbaf91a042c6a70bacf20a5ac85/java/src/org/openqa/selenium/support/ui/ExpectedCondition.java#L34-L36

titusfortner commented 10 months ago

Do we allow the native Java interface, or would we need to create a separate implementation for it. I'm hoping to pull it out of core Selenium code entirely for Selenium 5, so maybe we don't need to touch it and can spin it off at that time.

joerg1985 commented 10 months ago

It should be working fine with the native java interface.

joerg1985 commented 9 months ago

I just pushed a commit (42cc35585b8a60f722c67d34fdc877e3b4c2c89b) to remove the usage of guava from the browser bindings.

These are left areas are left:

titusfortner commented 8 months ago

@joerg1985 I'm going to remove the milestone on this one since there's no need for a specific timeframe on it. Are you planning to continue working on this? Thanks.

diemol commented 8 months ago

Having a single issue for this is helpful but not so actionable.

@joerg1985, if you have identified where we can make code changes, I suggest creating an issue for each place you found and adding instructions for the implementation details. With that, anyone could jump in and help. Then, closing this issue.

joerg1985 commented 8 months ago

Yes i have planned to work on this as soon as possible. I think i will continue after christmas. I will split this in multiple tasks the next days.

joerg1985 commented 7 months ago

Okay, i had a look and there is not much left, but there is one question how to remove the use of com.google.common.net.HttpHeaders and com.google.common.net.MediaType before i could split the issue or further implement it.

I would suggest to create two enums to replace them org.openqa.selenium.remote.http.HttpHeader and org.openqa.selenium.remote.http.MediaType. @diemol @titusfortner are you okay with that?

titusfortner commented 7 months ago

I'm pretty much ok with anything that doesn't require users to change their code. 😂

joerg1985 commented 7 months ago

I just created the less-guava branch which removes all easy to replace guava uses. I would merge this to main, after the release of 4.16.0, if the new class org.openqa.selenium.remote.http.HttpHeader is okay. After this is merged it should be easier to split this ticket, as there are much less areas left.

diemol commented 7 months ago

I just created the less-guava branch which removes all easy to replace guava uses. I would merge this to main, after the release of 4.16.0, if the new class org.openqa.selenium.remote.http.HttpHeader is okay. After this is merged it should be easier to split this ticket, as there are much less areas left.

I don't have a strong opinion. I believe we can have those enums, and hopefully documenting in the code why we have them. Thanks!

joerg1985 commented 7 months ago

@diemol @titusfortner i have just pushed a commit to replace most of the usages, these tasks are left:

Have to create the tickets to replace these usages the next days.

joerg1985 commented 7 months ago

Ok i was not able to describe all changes to remove the FileBackedOutputStream and CountingOutputStream, as this is not that trivial and i had to check a lot of things if it is possible to remove it.

So i ended in creating a PR with it, sorry for this ;)

joerg1985 commented 7 months ago

Use of guava in org.openqa.selenium.support.ui.FluentWait has been removed.

titusfortner commented 6 months ago

Hey @joerg1985 what is the status?

@diemol should we need to announce that we will be removing guava support for a specific release? Not sure the right way to communicate this.

diemol commented 6 months ago

I don't think so, we should not be exposing Guava functionality.

joerg1985 commented 6 months ago

@titusfortner still waiting for the review of #13308 by @shs96c

After the PR the is merged there is only one use of guava left (the breaking change in org.openqa.selenium.support.ui.ExpectedCondition).

titusfortner commented 6 months ago

Alternative option... We don't import Support package by default with selenium-java and keep the guava dependency in that package? Or is that a Selenium 5 level change? :)

titusfortner commented 6 months ago

I don't think so, we should not be exposing Guava functionality.

Yes, but if someone is using Selenium they have guava as a transitive dependency. They may be using it without having an explicit dependency, so updating Selenium will cause their code to break. I don't know if that should be called out ahead of time, or just part of the release notes.

krmahadevan commented 6 months ago

In my personal opinion transitive dependencies should not be something that end users should rely on because they are being brought in as transitive dependencies to solve some programming ask by selenium. Selenium would be free to make any changes to how those functionalities are delivered and in the process it should also be able to add/remove any dependencies that it needs.

An end user project that requires guava should explicitly depend on it and not rely on transitive dependencies (Which is always prone to issues especially if one has multiple libraries that bring in multiple versions of the same library, and thus causing dependency conflicts)

joerg1985 commented 4 months ago

@diemol @titusfortner Now is only one use of guava left is the breaking change in org.openqa.selenium.support.ui.ExpectedCondition would be greate to have this completed in 4.19.0.

diemol commented 4 months ago

Is there a PR for it?

joerg1985 commented 4 months ago

No, i did not know how to proper document this breaking change in the release notes. It is just removing the guava interface from the selenium interface and remove the guava from the bazel file.

diemol commented 4 months ago

I remember now. We need to write a blog post about it, announce it, and then make the change because we cannot just deprecate the interface or do something similar.

diemol commented 4 months ago

In the end, this helps users because there have been countless issues of people reporting dependency conflicts.