feat(check): experiment with getting tsc trace generation working #31252
Conversation
| import * as perf_hooks from "node:perf_hooks"; | ||
| var __filename = "sys.js"; | ||
| var __dirname = "/tmp"; | ||
| var require = (m) => { |
There was a problem hiding this comment.
this was easier than getting require working
nathanwhit
force-pushed
the
ts-generate-trace
branch
from
ed362bc to
66a6214
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR experiments with enabling TypeScript compiler trace generation to help diagnose type-checking performance issues (ref issue #14256). The changes modify the runtime and compiler infrastructure to support TSC's generateTrace feature.
- Introduces an
AllowAllpermissions struct to replace the restrictivePermissionsstub for snapshot extensions - Switches from
JsRuntimetoMainWorkerfor executing TSC, enabling proper permission setup - Enables TypeScript trace generation by passing
generateTraceoption to the compiler
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| runtime/snapshot_info.rs | Adds AllowAll permissions implementation and replaces Permissions with AllowAll in snapshot extensions initialization |
| runtime/js/99_main.js | Adds TypeScript-related ops to the list of ops not imported from snapshot |
| cli/tsc/js.rs | Replaces JsRuntime with MainWorker for TSC execution and adds debug logging |
| cli/tsc/99_main_compiler.js | Configures TypeScript compiler with generateTrace option and adds trace reporting |
| cli/tsc/97_ts_host.js | Imports TypeScript module for trace functionality |
| cli/tsc/00_typescript.js | Adds Node.js module shims and exports TypeScript functionality for trace generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ..Default::default() | ||
| }, | ||
| ); | ||
| eprintln!("HERE"); |
There was a problem hiding this comment.
Debug logging statements should be removed before merging. These eprintln! statements on lines 381, 391, 401, and 405 appear to be temporary debugging output and should be cleaned up.
| console.log(">>> exec start", { rootNames }); | ||
| console.log(config); |
There was a problem hiding this comment.
Changed from debug() to console.log(). This appears to be for debugging purposes. Consider reverting to use the existing debug() function to maintain consistency with the codebase's debugging patterns, or remove if this was temporary debugging output.
| console.log(">>> exec start", { rootNames }); | |
| console.log(config); | |
| debug(">>> exec start", { rootNames }); | |
| debug(config); |
| const ret = system === sys && compilerOptions.generateTrace; | ||
| console.log("canTrace", ret); | ||
| return ret; |
There was a problem hiding this comment.
Debug console.log statement should be removed before merging. This appears to be temporary debugging output added to verify trace generation.
| /* let mut runtime = JsRuntime::new(RuntimeOptions { | ||
| extensions, | ||
| create_params: create_isolate_create_params(&crate::sys::CliSys::default()), | ||
| startup_snapshot: deno_snapshots::CLI_SNAPSHOT, | ||
| extension_code_cache, | ||
| ..Default::default() |
There was a problem hiding this comment.
Commented-out code should be removed. If this old implementation needs to be preserved for reference, it should be documented in a commit message or issue instead.
| if (config.compilerOptions) { | ||
| Object.assign(config.compilerOptions, { generateTrace: tracePath }); | ||
| } else { | ||
| // config.compilerOptions = { generateTrace: tracePath }; |
There was a problem hiding this comment.
Remove commented-out code. If this alternative approach is needed for reference, document it in the PR description or an issue instead.
| // config.compilerOptions = { generateTrace: tracePath }; | |
| const { options, errors: configFileParsingDiagnostics } = ts | ||
| .convertCompilerOptionsFromJson(config, ""); | ||
| .convertCompilerOptionsFromJson(config, ""); |
There was a problem hiding this comment.
[nitpick] Indentation changed from proper alignment to inconsistent formatting. The opening . should align with the previous line's indentation pattern for code consistency.
| .convertCompilerOptionsFromJson(config, ""); | |
| .convertCompilerOptionsFromJson(config, ""); |
1 participant
ref #14256