Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow LocalNode to be extended for customisation #11718

Merged

Conversation

krmahadevan
Copy link
Contributor

LocalNode currently has a private constructor
Which prevents it from being extended for customisation. Exposing the constructor for classes extending it.

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@krmahadevan krmahadevan requested a review from diemol March 1, 2023 04:28
@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (63e8543) 54.65% compared to head (c684edb) 54.65%.

❗ Current head c684edb differs from pull request most recent head b7cb793. Consider uploading reports for the commit b7cb793 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##            trunk   #11718   +/-   ##
=======================================
  Coverage   54.65%   54.65%           
=======================================
  Files          85       85           
  Lines        5643     5643           
  Branches      244      244           
=======================================
  Hits         3084     3084           
  Misses       2315     2315           
  Partials      244      244           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage? Why not keep it the way it is? Folks can still extend Node, right?

@krmahadevan
Copy link
Contributor Author

What is the advantage? Why not keep it the way it is? Folks can still extend Node, right?

@diemol for most of the scenarios, a user should be perfectly fine in just extending Node and then wrapping a LocalNode within this extended class (like what we show in the documentation as well) but I noticed a user does not get calls when the session is quit using this approach because:

  1. A decorated node (A class that essentially extends Node but internally delegates calls to a LocalNode object) when it encounters a delete session request via executeWebDriverCommand will executeWebDriverCommand() on the local node object that it has (as seen here )
  2. The implementation in LocalNode.executeWebDriverCommand() is such that, it checks if the command is a delete session and it internally calls LocalNode.stop(SessionId id). See here
  3. Now this call from (2) never invokes the stop(SessionId id) on our Decorated node.

Owing to this, a user would never be able to eavesdrop on stop(Session) via a decorated node approach. So they will have to duplicate code from LocalNode completely to be able to tap into stop session events.

If we allowed users to extend LocalNode then they can easily just override the stop(Session) method in their child class and then include their custom logic as well.

Please let me know if that answers your question.

LocalNode currently has a private constructor 
Which prevents it from being extended for customisation.
Exposing the constructor for classes extending it.
@diemol diemol force-pushed the allow_local_node_to_be_extendible branch from b7cb793 to 7ae41f7 Compare March 1, 2023 10:01
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thank you!

@diemol diemol merged commit 906baa8 into SeleniumHQ:trunk Mar 1, 2023
@krmahadevan krmahadevan deleted the allow_local_node_to_be_extendible branch March 1, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants