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

[bazel + js] Allow bazel build //javascript/... to work #13893

Merged
merged 3 commits into from
May 1, 2024

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented May 1, 2024

Type

enhancement


Description

  • Enhanced the Bazel build configuration for the TypeScript project in javascript/grid-ui to support JavaScript and resolve JSON modules.
  • Fixed a JSON syntax error in the TypeScript configuration file.
  • Refactored the Bazel build setup for javascript/node/selenium-webdriver to use js_library for better management of JavaScript files and tests.

Changes walkthrough

Relevant files
Enhancement
BUILD.bazel
Enhance TypeScript Build Configuration for JavaScript Support

javascript/grid-ui/BUILD.bazel

  • Enabled JavaScript support in TypeScript project configuration.
  • Included JSON module resolution.
  • Updated the assets inclusion pattern.
  • +2/-1     
    BUILD.bazel
    Refactor and Enhance JavaScript Library Handling in Bazel

    javascript/node/selenium-webdriver/BUILD.bazel

  • Introduced js_library rules to manage JavaScript sources and tests
    more effectively.
  • Reorganized test and source file handling using js_library.
  • Improved package and test configuration for better modularity.
  • +57/-33 
    Bug fix
    tsconfig.json
    Fix JSON Syntax in TypeScript Configuration                           

    javascript/grid-ui/tsconfig.json

    • Removed trailing comma to fix JSON syntax.
    +1/-1     

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

    @shs96c shs96c marked this pull request as ready for review May 1, 2024 10:39
    Copy link

    PR Description updated to latest commit (096978d)

    Copy link

    PR Review

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across Bazel build files and configuration files, which requires understanding of Bazel, JavaScript project structures, and TypeScript configurations. The changes are not overly complex but are spread across several files and functionalities.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Possible Bug: The removal of the exclusion for "src/assets/**/*" in the glob pattern might unintentionally include files that should not be part of the build process. This could lead to unexpected behavior or bloated builds.

    Configuration Error: The removal of the comma in the tsconfig.json might be unintentional and could potentially lead to syntax errors or misinterpretation of the file structure in some parsers, although it appears correct in standard JSON syntax.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Enable source maps for better debugging capabilities.

    Reconsider setting "sourceMap": false in the TypeScript configuration. Source maps are
    crucial for debugging, as they map the compiled code back to the original source code,
    which is especially useful in production environments for error tracking and debugging.

    javascript/grid-ui/tsconfig.json [25]

    -"sourceMap": false
    +"sourceMap": true
     
    Best practice
    Prevent asset files from being treated as source files.

    Consider adding back the exclusion of "src/assets//*" in the glob function for SRCS.
    Excluding asset files can prevent them from being mistakenly considered as source files,
    which could potentially lead to build errors or unexpected behavior.**

    javascript/grid-ui/BUILD.bazel [33-36]

     exclude = [
         "src/simple.ts",
         "src/**/*.test.tsx",
    +    "src/assets/**/*",
     ],
     
    Security
    Validate JSON files when enabling JSON module resolution.

    Setting resolve_json_module = True in ts_project allows importing of JSON modules. Ensure
    that all JSON files used in the project are valid and secure, as importing JSON modules
    can potentially introduce security risks if the JSON data is manipulated.

    javascript/grid-ui/BUILD.bazel [58]

    -resolve_json_module = True
    +resolve_json_module = True  # Ensure JSON files are validated
     
    Maintainability
    Improve file organization and build performance by excluding irrelevant files.

    The js_library named "prod-src-files" includes a wide range of JavaScript files and
    directories. Consider adding more specific exclusions or organizing files better to ensure
    that only relevant source files are included, which can improve build performance and
    maintainability.

    javascript/node/selenium-webdriver/BUILD.bazel [24-40]

     srcs = [
         "CHANGES.md",
         "README.md",
         "package.json",
     ] + glob([
         "*.js",
         "example/*.js",
         "http/*.js",
         "io/*.js",
         "lib/*.js",
         "net/*.js",
         "remote/*.js",
         "testing/*.js",
         "devtools/*.js",
         "common/*.js",
         "bidi/*.js",
    -]),
    +], exclude=["**/deprecated/**"]),
     
    Performance
    Refine test data inclusion to improve build efficiency and clarity.

    The js_library named "test-data" includes a broad range of directories. To avoid including
    unnecessary test artifacts or data, refine the glob patterns or exclusions to better
    target only the necessary test resources.

    javascript/node/selenium-webdriver/BUILD.bazel [104-112]

     srcs = glob(
         [
    -        "lib/test/**",
    -        "test/**",
    +        "lib/test/data/**",  # More specific path
    +        "test/data/**",      # More specific path
         ],
         exclude = [
             "test/**/*_test.js",
         ],
     ),
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    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.

    None yet

    2 participants