refactor: rewrite gen-esbuild-test in TypeScript#7376
Merged
graphite-app[bot] merged 1 commit intomainfrom Dec 8, 2025
Merged
Conversation
Member
Author
How to use the Graphite Merge QueueAdd the label graphite: merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
This was referenced Dec 8, 2025
✅ Deploy Preview for rolldown-rs canceled.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR converts the gen-esbuild-test script from JavaScript to TypeScript, improving type safety for a script that generates test cases from esbuild's Go test files. The conversion includes proper TypeScript interfaces, type annotations, and integration with the TypeScript configuration.
Key changes:
- Added TypeScript type definitions and interfaces (
FileEntry,JsConfig,Config,TestSuiteName) - Converted JSDoc comments to TypeScript type annotations throughout all functions
- Updated imports to use type-only imports where appropriate (e.g.,
type Node as SyntaxNode)
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| scripts/tsconfig.json | Added misc/**/*.ts to include TypeScript files in the misc directory for type checking |
| scripts/package.json | Updated script entry to use .ts extension for the gen-esbuild-test command |
| scripts/misc/gen-esbuild-test.ts | Complete rewrite from JavaScript to TypeScript with type annotations, interfaces, and improved documentation |
Comments suppressed due to low confidence (11)
scripts/misc/gen-esbuild-test.ts:408
- The
JsConfiginterface declaresfilesandentryPathsas required properties, but the object is initialized withObject.create(null)which doesn't have these properties. This causes a type mismatch. Either:
- Make these properties optional in the interface:
files?: FileEntry[]andentryPaths?: string[], or - Initialize with proper defaults:
const jsConfig: JsConfig = { files: [], entryPaths: [], options: undefined }
Option 1 is recommended since the code already handles undefined/missing values (e.g., line 441 uses ??)
scripts/misc/gen-esbuild-test.ts:102
- The
optionsproperty is typed asvoidwhich is incorrect. Thevoidtype is typically used for function return types that don't return a value, not for object properties. Consider usingundefinedoranyinstead, or define a proper type for the options if they exist.
scripts/misc/gen-esbuild-test.ts:405 - Variable name uses snake_case (
bundle_field_list) which is inconsistent with the codebase's camelCase convention. Rename tobundleFieldListto match the style used throughout the codebase (e.g.,fileList,snapfileList,diffList).
scripts/misc/gen-esbuild-test.ts:124 - The type assertion
as TestSuiteNameis unsafe. If the user provides an invalid suite name, the assertion will hide the error and allow invalid values to propagate. Add validation after reading the argument:
const suiteName = process.argv[2];
if (!suites[suiteName as TestSuiteName]) {
throw new Error(`Invalid test suite name. Must be one of: ${Object.keys(suites).join(', ')}`);
}
const SUITE_NAME = suiteName as TestSuiteName;scripts/misc/gen-esbuild-test.ts:194
- The non-null assertions
child!are unnecessary here. TheforEachcallback always receives non-null items from the array. ThenamedChildrenproperty returns an array, and iterating over it guarantees eachchildparameter is defined. Remove the!operators on lines 193 and 194.
scripts/misc/gen-esbuild-test.ts:276 - The non-null assertions
child!are unnecessary here. TheforEachcallback receives non-null array items. Remove the!operators on lines 269, 272, and 276.
scripts/misc/gen-esbuild-test.ts:308 - The non-null assertion
child!is unnecessary. TheforEachcallback receives non-null array items. Remove the!operator.
scripts/misc/gen-esbuild-test.ts:388 - The non-null assertion on
tree.parse(source)!is potentially unsafe. Theparsemethod can return null if parsing fails. Consider adding proper error handling instead of using a non-null assertion, or at least add a check to ensure the result is not null before proceeding.
scripts/misc/gen-esbuild-test.ts:413 - Accessing
jsConfig.fileswithout checking if it exists. SincejsConfigis created withObject.create(null)and properties are set conditionally inprocessKeyElement, this property might be undefined. Add a check or provide a default:const fileList = jsConfig.files ?? []
scripts/misc/gen-esbuild-test.ts:362 - The return type
Promise<void> | undefinedis problematic. When the function returns early (line 362), it returnsundefinedimplicitly, but the caller on line 384 usesawaitwhich expects a Promise. This should either:
- Always return a Promise:
return Promise.resolve()on line 362, or - Remove the await on line 384 and handle the conditional promise differently
Option 1 is recommended for consistency: change line 362 to return Promise.resolve(); and update the return type to Promise<void>
scripts/misc/gen-esbuild-test.ts:440
- Using
Object.create({})to create the config object is unusual.Object.create({})creates an object that inherits from the empty object prototype, which is semantically odd. Use either:
{}for a plain object literalObject.create(null)for a truly empty object with no prototype- Properly initialize with the expected structure:
{ input: [] }
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hyf0
approved these changes
Dec 8, 2025
Contributor
Merge activity
|
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 8, 2025
Rewrote the gen-esbuild-test script in TypeScript with AI.
graphite-app
bot
force-pushed
the
12-07-chore_rewrite_gen-esbuild-test_in_typescript
branch
from
6f3f542 to
5f05d79
Compare
graphite-app bot
pushed a commit
that referenced
this pull request
Dec 8, 2025
Rewrote the gen-esbuild-test script in TypeScript with AI.
graphite-app
bot
force-pushed
the
12-07-chore_rewrite_gen-esbuild-test_in_typescript
branch
from
5f05d79 to
795b83e
Compare
Rewrote the gen-esbuild-test script in TypeScript with AI.
graphite-app
bot
force-pushed
the
12-07-chore_rewrite_gen-esbuild-test_in_typescript
branch
from
795b83e to
b1a619b
Compare
graphite-app
bot
deleted the
12-07-chore_rewrite_gen-esbuild-test_in_typescript
branch
This was referenced Dec 10, 2025
shulaoda
added a commit
that referenced
this pull request
Dec 11, 2025
## [1.0.0-beta.54] - 2025-12-11 ⏱️ Plugin Timing Analysis - Added `checks.pluginTimings` option to emit warnings when plugins significantly impact build performance - More details: https://rolldown.rs/options/checks#plugintimings <img width="834" height="119" alt="image" src="https://github.com/user-attachments/assets/e819f906-6192-4503-8d5c-854fa92ecb45" /> > [!WARNING] > For hooks using `ctx.resolve()` or `ctx.load()`, the reported time includes waiting for other plugins, which may overestimate that plugin's actual cost. > > Additionally, since plugin hooks execute concurrently, the statistics represent accumulated time rather than wall-clock time. The measured duration also includes Rust-side processing overhead, Tokio async scheduling overhead, NAPI data conversion overhead, and JavaScript event loop overhead. ### 🚀 Features - rolldown_plugin_vite_reporter: add newline after build summary for better log separation (#7458) by @shulaoda - plugin: collect plugin hook execution timings (#7364) by @shulaoda - test-dev-server: reload page when detecting hmr reload message (#7422) by @hyf0 - rolldown_plugin_vite_dynamic_import_vars: add transform-based v2 implementation (#7400) by @shulaoda - rolldown_plugin_vite_import_glob: add transform-based v2 implementation (#7394) by @shulaoda - rolldown_plugin_vite_wasm_helper: add v2 implementation (#7402) by @shulaoda - expose error location and context fields on JS API error objects (#7341) by @Copilot - add `CIRCULAR_REEXPORT` error (#7337) by @Copilot - support emit prebuilt chunk (#7277) by @Copilot ### 🐛 Bug Fixes - preserve object key order when parse json with serde_json (#7443) by @IWANABETHATGUY - improve JSON parsing with serde_json to emit proper diagnostic (#7442) by @IWANABETHATGUY - deconflict external symbols in CJS modules (#7447) by @IWANABETHATGUY - rolldown_plugin_vite_transform,rolldown_plugin_vite_resolve: enable `yarnPnp` option when pnp is detected (#5791) by @sapphi-red - skip deconflicting top-level symbols for CJS modules (#7425) by @IWANABETHATGUY - rolldown_plugin_esm_external_require: run resolveId hook before other plugins (#7426) by @shulaoda - avoid duplicate underscores in legitimized identifiers (#7418) by @IWANABETHATGUY - use `process.on('exit')` instead of `signal-exit` on webcontainers (#7421) by @sapphi-red - dev: remove `imports` when an import is removed (#7348) by @sapphi-red - bench: access latency.mean instead of mean in tinybench result (#7417) by @shulaoda - path compression in symbol linking (#7392) by @IWANABETHATGUY - rolldown_plugin_vite_resolve: add RwLock to avoid clearing cache while resolving (#7386) by @sapphi-red - handle JSON prototype properties correctly (#7383) by @IWANABETHATGUY - preserve entry signature strict chunk merging (#7343) by @IWANABETHATGUY - support eliminating multiple unused dynamic imports in a single statement (#7361) by @IWANABETHATGUY - eliminate unreachable dynamic entry (#7356) by @IWANABETHATGUY - `NormalizedInputOptions#cwd` should always exists (#7360) by @hyf0 - cli: support multiple comma-separated define arguments (#7340) by @Copilot - remove redundant symbol param in __reExport runtime helper (#7346) by @IWANABETHATGUY - always use __export for empty namespace objects when symbols enabled (#7345) by @IWANABETHATGUY - relax the restriction of preserveEntrySignatures when merging chunks (#7339) by @IWANABETHATGUY ### 🚜 Refactor - builtin-plugin: make config parameter required for vite plugins (#7451) by @shulaoda - move esbuild test related scripts to `scripts/src/esbuild-tests` (#7377) by @sapphi-red - rewrite gen-esbuild-test in TypeScript (#7376) by @sapphi-red - remove unnecessary Option in current_stmt_idx (#7359) by @IWANABETHATGUY - move more code into chunk optimizer (#7335) by @IWANABETHATGUY ### 📚 Documentation - checks: clarify pluginTimings measures CPU time due to concurrent execution (#7448) by @shulaoda - checks: add accuracy note to pluginTimings documentation (#7441) by @shulaoda - development-guide: add esbuild test description (#7439) by @sapphi-red - checks: expand `pluginTimings` documentation with detection mechanism (#7428) by @shulaoda - add checks options documentation (#7427) by @shulaoda - development-guide: add test262 integration test description (#7430) by @sapphi-red - guide: fix grammer in getting-started (#7331) by @jakeparis ### 🧪 Testing - rollup-tests: make `--grep` work and document it (#7431) by @sapphi-red - add a way to run some test262 test cases by name (#7429) by @sapphi-red - rolldown_plugin_vite_dynamic_import_vars: add test cases for v2 implementation (#7401) by @shulaoda - rolldown_plugin_vite_import_glob: add test cases for v2 implementation (#7395) by @shulaoda - triage new esbuild tests (#7405) by @sapphi-red - hmr: delete file used (#5933) by @sapphi-red - watch: add `watchChange` hook `create` / `delete` tests (#7349) by @sapphi-red - hmr: delete file not used anymore (#5932) by @sapphi-red - triage esbuild failed reasons (#7391) by @sapphi-red - change esbuild tests stats and diff generation (#7379) by @sapphi-red - dev: add `continuous generate hmr patch` and `debounce bundle` tests (#7368) by @hyf0 - add new esbuild tests (#7353) by @sapphi-red ### ⚙️ Miscellaneous Tasks - setup Node before updating snapshots for test262 update (#7435) by @sapphi-red - run tests when submodules are updated (#7455) by @sapphi-red - exclude NUL path pattern to prevent crash on Windows (#7450) by @IWANABETHATGUY - run cargo-test for esbuild script changes and update esbuild snapshot (#7440) by @sapphi-red - jsx_import_meta tests to `ignoreReasons` due to architectural limitation (#7434) by @Copilot - deps: update dependency oxlint-tsgolint to v0.8.5 (#7433) by @renovate[bot] - update test dependencies automatically (#7432) by @sapphi-red - remove unused code (#7420) by @IWANABETHATGUY - add back `just update-esbuild-diff` and add it to CI (#7404) by @sapphi-red - deps: update oxc-resolver to 11.15.0 (#7415) by @shulaoda - deps: update rust crate napi to v3.7.0 (#7393) by @renovate[bot] - fix esbuild compatibility metrics calculation (#7390) by @sapphi-red - support `tsconfig: true` in rust tests (#7389) by @sapphi-red - hide oxc runtime in snapshots (#7388) by @sapphi-red - deps: update oxc to v0.102.0 (#7385) by @camc314 - improve esbuild tests to download snapshots automatically (#7378) by @sapphi-red - deps: update dependency tinybench to v6 (#7371) by @renovate[bot] - deps: update actions/checkout action to v6 (#7370) by @renovate[bot] - deps: update rust crates (#7367) by @renovate[bot] - deps: update dependency oxlint to v1.32.0 (#7374) by @renovate[bot] - deps: update dependency oxlint-tsgolint to v0.8.4 (#7373) by @renovate[bot] - node/test-dev-server: support using custom html file (#7357) by @hyf0 - test: setup browser-based e2e test for `test-dev-server` (#7351) by @hyf0 - pin pnpm to version 10.23.0 (#7369) by @IWANABETHATGUY - deps: update dependency rolldown-plugin-dts to v0.18.3 (#7372) by @renovate[bot] - deps: update github-actions (#7365) by @renovate[bot] - deps: update npm packages (#7366) by @renovate[bot] - polish the comments about `SymbolRefFlags` (#7358) by @IWANABETHATGUY - deps: update dependency rolldown-plugin-dts to v0.18.2 (#7355) by @renovate[bot] - fix esbuild test generation script (#7352) by @sapphi-red - apply cargo shear suggestion (#7347) by @IWANABETHATGUY - deps: update oxc to v11.15.0 (#7344) by @renovate[bot] - deps: update dependency @napi-rs/cli to v3.5.0 (#7338) by @renovate[bot] - deps: update dependency oxlint to v1.31.0 (#7305) by @renovate[bot] - update rollup-tests results (#7333) by @sapphi-red - update rollup-tests Rollup version (#7332) by @sapphi-red - add ignore config for test262 submodule (#7326) by @Copilot ### ❤️ New Contributors * @jakeparis made their first contribution in [#7331](#7331) Co-authored-by: shulaoda <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rewrote the gen-esbuild-test script in TypeScript with AI.