Skip to content

Conversation

vitalets
Copy link
Contributor

@vitalets vitalets commented Sep 12, 2025

User description

🔗 Related Issues

Related to #13993

💥 What does this PR do?

Adds asMap() method to the Header class.

🔧 Implementation Notes

The implementation is similar to the other classes.

💡 Additional Considerations

The same change is included in #15571.
However, merging it may take some time since it introduces new concepts and still in draft. Meanwhile, the existing code already expects the Header class to implement an asMap() method.

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix


Description

  • Add missing asMap() method to Header class

  • Fix existing code that expects this method

  • Enhance test with response headers and body validation


Diagram Walkthrough

flowchart LR
  A["Header class"] --> B["Add asMap() method"]
  B --> C["Return Map with name/value"]
  D["Test enhancement"] --> E["Add headers to response"]
  E --> F["Validate response content"]
Loading

File Walkthrough

Relevant files
Bug fix
networkTypes.js
Add asMap() method to Header class                                             

javascript/selenium-webdriver/bidi/networkTypes.js

  • Add asMap() method to Header class
  • Method returns Map with 'name' and 'value' entries
  • Value is converted using BytesValue.asMap()
+11/-0   
Tests
network_commands_test.js
Enhance network test with response validation                       

javascript/selenium-webdriver/test/bidi/network_commands_test.js

  • Import Header and BytesValue classes
  • Enhance test with response headers and body
  • Add validation for response content
+10/-1   

@CLAassistant
Copy link

CLAassistant commented Sep 12, 2025

CLA assistant check
All committers have signed the CLA.

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

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

5678 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Investigate and resolve "ConnectFailure" on subsequent ChromeDriver instantiations.
  • Ensure stable creation of multiple ChromeDriver instances without connection failures.
  • Provide guidance or fixes to avoid/refuse connection errors after the first instance.

Requires further human verification:

  • Reproduce the issue in the specified Ubuntu/Chrome/ChromeDriver environment to validate behavior.

1234 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Ensure click() triggers JavaScript in href on Firefox as in 2.47.1.

Requires further human verification:

  • Manual browser testing on affected Firefox versions to verify click() behavior.

13993 - Partially compliant

Compliant requirements:

Non-compliant requirements:

  • Provide a high-level BiDi network API accessible from Driver (e.g., driver.network()) across bindings.
  • Implement request handlers: add/remove/clear.
  • Implement response handlers: add/remove/clear.
  • Implement authentication handlers: add/remove/clear.
  • Consider returning IDs from "add" methods to simplify "remove".
  • Keep the API consistent (beta) across Java, NodeJS, Python, Ruby, .NET.

Requires further human verification:

  • Cross-language API parity and ergonomics cannot be validated in this PR.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Type Mismatch

JSDoc states the method returns Map<string, string>, but 'value' is assigned an object derived from BytesValue.asMap(). Align the return type and/or structure (e.g., Map<string, any> or convert consistently).

/**
 * Converts the Header to a map.
 * @returns {Map<string, string>} A map representation of the Header.
 */
asMap() {
  const map = new Map()
  map.set('name', this._name)
  map.set('value', Object.fromEntries(this._value.asMap()))
  return map
}  
Serialization Risk

Mixing a Map (outer) with a plain object (inner 'value') can cause inconsistent serialization. Consider converting both to plain objects or keep Maps consistently and handle nested conversion where serialization occurs.

/**
 * Converts the Header to a map.
 * @returns {Map<string, string>} A map representation of the Header.
 */
asMap() {
  const map = new Map()
  map.set('name', this._name)
  map.set('value', Object.fromEntries(this._value.asMap()))
  return map
}  
Import Check

The test uses ProvideResponseParameters in the new code path; ensure it is imported in this file to avoid a ReferenceError at runtime.

await network.provideResponse(new ProvideResponseParameters(event.request.request)
  .statusCode(200)
  .headers([
    new Header("Content-Type", new BytesValue("string", "text/html"))
  ])
  .body(new BytesValue("string", "<html><body>Hello world</body></html>"))
)
counter = counter + 1

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Return plain object, not Map

Return a plain object instead of a Map so it can be JSON-serialized when
building BiDi payloads. Maps stringify to empty objects, breaking protocol
requests.

javascript/selenium-webdriver/bidi/networkTypes.js [76-85]

 /**
- * Converts the BytesValue to a map.
- * @returns {Map<string, string>} A map representation of the BytesValue.
+ * Converts the BytesValue to a plain object.
+ * @returns {{type: string, value: string}} A plain object representation of the BytesValue.
  */
 asMap() {
-  const map = new Map()
-  map.set('type', this._type)
-  map.set('value', this._value)
-  return map
+  return {
+    type: this._type,
+    value: this._value,
+  }
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies that JSON.stringify() on a Map object produces an empty object {}, which would lead to incorrect BiDi protocol payloads. Changing BytesValue.asMap() to return a plain object is a critical fix for correct serialization.

High
Use plain object for serialization

Return a plain object for the header representation to ensure proper JSON
serialization. Also avoid mixing Map with plain objects to keep structure
consistent.

javascript/selenium-webdriver/bidi/networkTypes.js [123-132]

 /**
- * Converts the Header to a map.
- * @returns {Map<string, string>} A map representation of the Header.
+ * Converts the Header to a plain object.
+ * @returns {{name: string, value: {type: string, value: string}}} A plain object representation of the Header.
  */
 asMap() {
-  const map = new Map()
-  map.set('name', this._name)
-  map.set('value', Object.fromEntries(this._value.asMap()))
-  return map
+  return {
+    name: this._name,
+    value: this._value.asMap(),
+  }
 }
  • Apply / Chat
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly points out that returning a Map from Header.asMap() will cause serialization issues, as JSON.stringify(new Map()) results in '{}'. Returning a plain object is the correct approach for creating JSON payloads for the BiDi protocol.

High
  • More

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.

3 participants