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

[JS] Add initial TS config for migration (DO NOT MERGE) #15440

Draft
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

harsha509
Copy link
Member

@harsha509 harsha509 commented Mar 16, 2025

User description

Motivation and Context

This is a test commit to set up TypeScript configuration for the JavaScript
codebase migration.

Please do not Merge

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, Configuration changes


Description

  • Introduced TypeScript support with new configuration and file updates.

  • Refactored existing JavaScript files to TypeScript, adding type annotations.

  • Added a new proxy.ts module for WebDriver proxy configuration.

  • Updated Bazel build files to integrate TypeScript compilation.


Changes walkthrough 📝

Relevant files
Tests
1 files
upload_test.js
Removed unnecessary ESLint disable comments.                         
+2/-2     
Miscellaneous
1 files
pinnedScript.js
Removed ESLint disable comment for UUID generation.           
+1/-1     
Enhancement
2 files
virtual_authenticator.ts
Refactored to TypeScript with type annotations and interfaces.
+97/-65 
proxy.ts
Added new TypeScript module for WebDriver proxy configuration.
+190/-0 
Configuration changes
2 files
BUILD.bazel
Updated Bazel build file to support TypeScript compilation.
+70/-14 
tsconfig.json
Added TypeScript configuration file.                                         
+28/-0   
Dependencies
1 files
package.json
Added `@types/node` dependency for TypeScript support.     
+1/-0     
Additional files
2 files
proxy.js +0/-222 
pnpm-lock.yaml +321/-302

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Type Safety

    The manual() function accepts a bypass parameter as string[] but doesn't handle the case where it might be provided as a comma-separated string as mentioned in the function documentation.

    function manual({
      ftp,
      http,
      https,
      bypass,
    }: {
      ftp?: string
      http?: string
      https?: string
      bypass?: string[]
    }): ManualConfig {
      return {
        proxyType: Type.MANUAL,
        ftpProxy: ftp,
        httpProxy: http,
        sslProxy: https,
        noProxy: bypass,
      }
    }
    Build Configuration

    The ts_project configuration has allow_js set to False which might cause issues when migrating existing JS files to TypeScript incrementally.

    ts_project(
        name = "ts-compile",
        srcs = [":ts_sources"],
        allow_js = False,
        declaration = False,
        source_map = True,
        tsconfig = ":tsconfig",
        deps = [
            ":node_modules/@bazel/runfiles",
            ":node_modules/@types/node",
            ":node_modules/jszip",
            ":node_modules/tmp",
            ":node_modules/ws",
        ],
    )

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Generate TypeScript declaration files

    Enable TypeScript declaration file generation to provide better type information
    for consumers of the library. This is important for a migration to TypeScript as
    it allows other projects to benefit from the type definitions.

    javascript/node/selenium-webdriver/BUILD.bazel [77]

     # TypeScript compilation
     ts_project(
         name = "ts-compile",
         srcs = [":ts_sources"],
         allow_js = False,
    -    declaration = False,
    +    declaration = True,
         source_map = True,
         tsconfig = ":tsconfig",
         deps = [
             ":node_modules/@bazel/runfiles",
             ":node_modules/@types/node",
             ":node_modules/jszip",
             ":node_modules/tmp",
             ":node_modules/ws",
         ],
     )

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: Enabling declaration file generation is crucial for TypeScript libraries as it provides type information to consumers. This change would significantly improve the usability of the library for TypeScript users and is aligned with the PR's goal of TypeScript migration.

    Medium
    Enable strict type checking

    Enable TypeScript's strict mode to catch potential type errors early in the
    migration process. This will help identify issues that might otherwise only be
    discovered at runtime.

    javascript/node/selenium-webdriver/tsconfig.json [10-11]

     {
       "compilerOptions": {
         "target": "es2022",
         "module": "CommonJS",
         "moduleResolution": "node",
         "esModuleInterop": true,
         "allowJs": false, // Only process TypeScript files
         "sourceMap": true,
         "skipLibCheck": true,
    -    "strict": false,
    +    "strict": true,
         "declaration": false
       },
       "include": [
         "*.ts",
         "example/**/*.ts",
         "http/**/*.ts",
         "io/**/*.ts",
         "lib/**/*.ts",
         "lib/fedcm/**/*.ts",
         "net/**/*.ts",
         "remote/**/*.ts",
         "testing/**/*.ts",
         "devtools/**/*.ts",
         "common/**/*.ts",
         "bidi/**/*.ts"
       ],
       "exclude": ["node_modules"]
     }

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 7

    __

    Why: Enabling strict type checking would help catch potential type errors early in the TypeScript migration process, improving code quality and reducing runtime issues. This is a valuable change for a project transitioning to TypeScript.

    Medium
    Simplify redundant null check

    The userHandle() method has redundant null check logic. The method can be
    simplified to directly return the _userHandle property since it will return null
    if the property is null.

    javascript/node/selenium-webdriver/lib/virtual_authenticator.ts [186-189]

    -if (this._userHandle != null) {
    -  return this._userHandle
    -}
    -return null
    +return this._userHandle
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    __

    Why: The suggestion correctly identifies redundant null-checking logic. The method can be simplified to directly return this._userHandle since it will be null if the property is null. This is a minor improvement to code readability.

    Low
    Possible issue
    Make method static

    The fromDict method is defined as an instance method but appears to be used as a
    static factory method. It should be declared as static to match the pattern of
    other factory methods in the class.

    javascript/node/selenium-webdriver/lib/virtual_authenticator.ts [252-265]

    -fromDict(data: CredentialData): Credential {
    +static fromDict(data: CredentialData): Credential {
       const id = new Uint8Array(Buffer.from(data.credentialId, 'base64url'))
       const isResidentCredential = data.isResidentCredential
       const rpId = data.rpId
       const privateKey = Buffer.from(data.privateKey, 'base64url').toString('binary')
       const signCount = data.signCount
       let userHandle: Uint8Array | null = null
     
       if (data.userHandle) {
         userHandle = new Uint8Array(Buffer.from(data.userHandle, 'base64url'))
       }
     
       return new Credential(id, isResidentCredential, rpId, userHandle, privateKey, signCount)
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the fromDict method should be static to match the pattern of other factory methods in the class. This is an important consistency improvement that aligns with the class design pattern and would prevent potential usage errors.

    Medium
    • More

    @harsha509
    Copy link
    Member Author

    harsha509 commented Mar 16, 2025

    @titusfortner titusfortner marked this pull request as draft March 17, 2025 00:37
    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