Skip to content

Conversation

ankianan
Copy link

@ankianan ankianan commented Sep 21, 2025

User description

🔗 Related Issues

Fixes #16339

💥 What does this PR do?

Implement asMap function for Header class

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Enhancement


Description

  • Add asMap() method to Header class in BiDi network types

  • Converts Header object to Map with name and value entries

  • Implements missing functionality for Header serialization


Diagram Walkthrough

flowchart LR
  A["Header class"] --> B["asMap() method"]
  B --> C["Map with name/value"]
  C --> D["Serialized Header data"]
Loading

File Walkthrough

Relevant files
Enhancement
networkTypes.js
Add Header asMap method                                                                   

javascript/selenium-webdriver/bidi/networkTypes.js

  • Add asMap() method to Header class
  • Returns Map with "name" and "value" entries
  • Value is converted using nested asMap() call
+11/-0   

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@selenium-ci selenium-ci added C-nodejs JavaScript Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Sep 21, 2025
Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

API Consistency

Confirm that returning a native Map from Header.asMap() aligns with surrounding BiDi type serializers; other types may return plain objects. Inconsistent return types could complicate consumers and serialization.

asMap() {
      return new Map([
          ["name", this.name],
          ["value", Object.fromEntries(this.value.asMap())]
      ]);
  };
Value Handling

Object.fromEntries(this.value.asMap()) assumes value.asMap() returns entries; verify BytesValue.asMap() exists and matches expected structure. If value can be null/primitive, add guards to prevent runtime errors.

    ["name", this.name],
    ["value", Object.fromEntries(this.value.asMap())]
]);
Trailing Semicolon/Method Syntax

The class method ends with }; which is unnecessary for class method definitions and may violate lint rules; consider removing the trailing semicolon.

asMap() {
      return new Map([
          ["name", this.name],
          ["value", Object.fromEntries(this.value.asMap())]
      ]);
  };

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Ensure consistent nested Map structures

In the Header.asMap method, remove the Object.fromEntries call to ensure the
nested value remains a Map, maintaining data structure consistency.

javascript/selenium-webdriver/bidi/networkTypes.js [128-131]

 return new Map([
     ["name", this.name],
-    ["value", Object.fromEntries(this.value.asMap())]
+    ["value", this.value.asMap()]
 ]);
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why: The suggestion correctly identifies that converting the nested Map from this.value.asMap() to an object using Object.fromEntries creates an inconsistent data structure, and the proposed change improves API consistency.

Low
  • More

@ankianan ankianan changed the title Update networkTypes.js Implements missing Header.asMap to return a Map Sep 21, 2025
Copy link
Member

@Delta456 Delta456 left a comment

Choose a reason for hiding this comment

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

Can you add a test for this?

@cgoldberg
Copy link
Member

@ankianan The linter is failing on this.
Can you run ./scripts/format.sh and then update your branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-devtools Includes everything BiDi or Chrome DevTools related C-nodejs JavaScript Bindings Review effort 2/5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: TypeError: header.asMap is not a function
5 participants