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

[bidi][js] Deprecate argument value wrapper class #14251

Merged
merged 2 commits into from
Jul 11, 2024

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Jul 11, 2024

User description

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.

PR Type

Enhancement, Tests


Description

  • Deprecated the ArgumentValue class in favor of using LocalValue methods directly.
  • Added a new static method createReferenceValue to the LocalValue class.
  • Renamed the toJson method to asMap in the LocalValue class.
  • Updated various parts of the codebase to remove the ArgumentValue class and use LocalValue directly.
  • Modified tests to reflect the changes in the usage of LocalValue.

Changes walkthrough 📝

Relevant files
Enhancement
argumentValue.js
Deprecate `ArgumentValue` class and update `asMap` method

javascript/node/selenium-webdriver/bidi/argumentValue.js

  • Deprecated ArgumentValue class.
  • Updated asMap method to use LocalValue.
  • +7/-1     
    protocolValue.js
    Add `createReferenceValue` method and rename `toJson` to `asMap`

    javascript/node/selenium-webdriver/bidi/protocolValue.js

  • Added createReferenceValue static method.
  • Renamed toJson method to asMap.
  • +5/-1     
    script.js
    Remove `ArgumentValue` import and use `LocalValue` directly

    javascript/node/selenium-webdriver/lib/script.js

  • Removed ArgumentValue import.
  • Updated code to use LocalValue directly.
  • +1/-2     
    Tests
    local_value_test.js
    Update tests to use `LocalValue` directly                               

    javascript/node/selenium-webdriver/test/bidi/local_value_test.js

  • Removed ArgumentValue import.
  • Updated tests to use LocalValue directly.
  • +14/-15 
    locate_nodes_test.js
    Update locate nodes tests to use `LocalValue` directly     

    javascript/node/selenium-webdriver/test/bidi/locate_nodes_test.js

  • Removed ArgumentValue import.
  • Updated tests to use LocalValue directly.
  • +1/-2     
    script_test.js
    Update script tests to use `LocalValue` directly                 

    javascript/node/selenium-webdriver/test/bidi/script_test.js

  • Removed ArgumentValue import.
  • Updated tests to use LocalValue directly.
  • +8/-8     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Deprecated Class
    The class ArgumentValue is deprecated and replaced by direct usage of LocalValue methods. Ensure that all references and dependencies are updated accordingly to prevent future maintenance issues.

    Method Addition
    New methods createReferenceValue and asMap are added to LocalValue. Review for consistency and correctness in implementation, especially in how asMap constructs the return object.

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add error handling in createReferenceValue to ensure input validity

    Add error handling in createReferenceValue to ensure that both handle and sharedId
    are valid before creating a new ReferenceValue. This can prevent potential runtime
    errors due to invalid inputs.

    javascript/node/selenium-webdriver/bidi/protocolValue.js [188-189]

     static createReferenceValue(handle, sharedId) {
    +  if (!handle || !sharedId) {
    +    throw new Error('Invalid handle or sharedId for creating ReferenceValue');
    +  }
       return new ReferenceValue(handle, sharedId)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling to ensure that both handle and sharedId are valid before creating a new ReferenceValue is crucial for preventing potential runtime errors, thus improving the robustness of the code.

    9
    Enhancement
    Remove the asMap method from ArgumentValue to enforce the use of LocalValue

    Since ArgumentValue is deprecated and its functionality is being replaced by
    LocalValue, consider removing the asMap method from ArgumentValue to prevent its
    usage and enforce migration to LocalValue.

    javascript/node/selenium-webdriver/bidi/argumentValue.js [31-36]

    -asMap() {
    -  if (this.value instanceof LocalValue) {
    -    return this.value.asMap()
    -  } else {
    -    // ReferenceValue
    -    return this.value.asMap()
    +// asMap method removed to enforce using LocalValue directly
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Removing the asMap method from a deprecated class enforces the migration to the new LocalValue class, which is a significant improvement in maintaining code consistency and preventing the use of deprecated functionality.

    8
    Add a deprecation warning in the ArgumentValue constructor

    Since the ArgumentValue class is deprecated, consider adding a console warning in
    the constructor to inform developers about its deprecation when an instance is
    created.

    javascript/node/selenium-webdriver/bidi/argumentValue.js [27-28]

     constructor(value) {
    +  console.warn('ArgumentValue is deprecated. Use LocalValue directly.');
       this.value = value
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding a console warning in the constructor of a deprecated class informs developers about its deprecation, which is a helpful enhancement for maintaining code quality and guiding developers towards using the recommended class.

    7
    Possible issue
    Add error handling for undefined type in asMap to improve method robustness

    Consider adding a default case or error handling in asMap to manage unexpected type
    values gracefully, which can improve the robustness of the method.

    javascript/node/selenium-webdriver/bidi/protocolValue.js [192-194]

     asMap() {
       let toReturn = {}
    +  if (!this.type) {
    +    throw new Error('Undefined type in LocalValue');
    +  }
       toReturn[TYPE_CONSTANT] = this.type
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for undefined type values in the asMap method improves the robustness and reliability of the method, ensuring that unexpected values are managed gracefully.

    8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant