bazel: make npm/node a dev_dependency, invisible to downstream#10098
bazel: make npm/node a dev_dependency, invisible to downstream#10098maliberty merged 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
The aspect_rules_js, rules_nodejs, and their use_extension calls (node toolchain, npm translate_lock) exist solely for JavaScript unit tests in src/web/test/. No production cc_library target depends on npm packages — the JS files are served from disk by the C++ web server. Changes: - Mark bazel_dep entries for aspect_rules_js and rules_nodejs as dev_dependency = True in MODULE.bazel - Mark node and npm use_extension calls as dev_dependency = True - Remove isolate = True from npm extension (unnecessary when dev_dependency, and it required --experimental_isolated_extension_usages in downstream consumers) - Move package.json and pnpm-lock.yaml from src/web/ to src/web/test/ - Move npm_link_all_packages from src/web/BUILD to src/web/test/BUILD - Replace js_library with a plain filegroup for cross-package JS file references (no @aspect_rules_js dependency in production BUILD) Downstream Bazel consumers no longer inherit npm/node infrastructure, eliminating the need for patches like openroad-remove-isolate.patch. Tested: - All 10 js_test targets in //src/web/test pass - Downstream consumer builds without any npm-related patches Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@hzeller FYI |
|
clang-tidy review says "All clean, LGTM! 👍" |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the JavaScript build configuration by moving npm dependencies to the test directory and marking them as development dependencies. It replaces the js_library target with a filegroup and updates test targets to use local node modules. A suggestion was made to use variables in the test BUILD file to reduce redundancy and improve maintainability.
Define JS_FILES and DOM_TEST_DATA variables to avoid repeating the same data and no_copy_to_bin lists across all js_test targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request refactors the JavaScript build and test infrastructure by moving NPM dependencies and the lockfile to the src/web/test directory and marking them as development dependencies. It also simplifies the js_test definitions in src/web/test/BUILD using shared variables. A review comment suggests restricting the visibility of the new js_files filegroup in src/web/BUILD to only the test package for better encapsulation.
| filegroup( | ||
| name = "js_files", | ||
| srcs = glob(["src/*.js"]), | ||
| ) |
There was a problem hiding this comment.
For better encapsulation, consider restricting the visibility of this filegroup. Since these JavaScript files are only used for tests in //src/web/test, a more specific visibility like visibility = ["//src/web/test:__pkg__"] would be more appropriate than the package's default visibility.
| filegroup( | |
| name = "js_files", | |
| srcs = glob(["src/*.js"]), | |
| ) | |
| filegroup( | |
| name = "js_files", | |
| srcs = glob(["src/*.js"]), | |
| visibility = ["//src/web/test:__pkg__"], | |
| ) |
maliberty
merged commit ed5a5bc
into
The-OpenROAD-Project:master
|
Thank you! |
2 participants
The aspect_rules_js, rules_nodejs, and their use_extension calls (node toolchain, npm translate_lock) exist solely for JavaScript unit tests in src/web/test/. No production cc_library target depends on npm packages — the JS files are served from disk by the C++ web server.
Changes:
Downstream Bazel consumers no longer inherit npm/node infrastructure, eliminating the need for patches like openroad-remove-isolate.patch.
Tested: