Re: [PR] fix: fix the pre-commit hook for tsc [superset]

Wed, 23 Jul 2025 01:53:59 -0700 


korbit-ai[bot] commented on code in PR #34275:
URL: https://github.com/apache/superset/pull/34275#discussion_r2224878697


##########
scripts/check-type.js:
##########
@@ -45,27 +49,94 @@ void (async () => {
   }
 
   const packageRootDir = await getPackage(packageArg);
-  const updatedArgs = removePackageSegment(remainingArgs, packageRootDir);
-  const argsStr = updatedArgs.join(" ");
+  const changedFiles = removePackageSegment(remainingArgs, packageRootDir);
 
-  const excludedDeclarationDirs = getExcludedDeclarationDirs(
-    excludeDeclarationDirArg
+  // Filter to only TypeScript files
+  const tsFiles = changedFiles.filter(file =>
+    /\.(ts|tsx)$/.test(file) && !DECLARATION_FILE_REGEX.test(file)
   );
+
+  console.log(`Type checking ${tsFiles.length} changed TypeScript files...`);
+
+  if (tsFiles.length === 0) {
+    console.log("No TypeScript files to check.");
+    exit(0);
+  }
+
+  // Decide strategy based on number of files
+  if (tsFiles.length > MAX_FILES_FOR_TARGETED_CHECK) {
+    console.log(`Too many files (${tsFiles.length} > 
${MAX_FILES_FOR_TARGETED_CHECK}), running full type check...`);
+    await runFullTypeCheck(packageRootDir, excludeDeclarationDirArg);
+  } else {
+    console.log(`Running targeted type check on ${tsFiles.length} files...`);
+    await runTargetedTypeCheck(packageRootDir, tsFiles, 
excludeDeclarationDirArg);
+  }
+})();
+
+/**
+ * Run full type check on the entire project
+ */
+async function runFullTypeCheck(packageRootDir, excludeDeclarationDirArg) {
+  const packageRootDirAbsolute = join(SUPERSET_ROOT, packageRootDir);
+  const tsConfig = getTsConfig(packageRootDirAbsolute);
+  // Use incremental compilation for better caching
+  const command = `--noEmit --allowJs --incremental --project ${tsConfig}`;
+
+  await executeTypeCheck(packageRootDirAbsolute, command);
+}
+
+/**
+ * Run targeted type check on specific files, with batching
+ */
+async function runTargetedTypeCheck(packageRootDir, tsFiles, 
excludeDeclarationDirArg) {
+  const excludedDeclarationDirs = 
getExcludedDeclarationDirs(excludeDeclarationDirArg);
   let declarationFiles = await getFilesRecursively(
-    packageRootDir,
+    join(SUPERSET_ROOT, packageRootDir),
     DECLARATION_FILE_REGEX,
     excludedDeclarationDirs
   );
   declarationFiles = removePackageSegment(declarationFiles, packageRootDir);
-  const declarationFilesStr = declarationFiles.join(" ");
 
   const packageRootDirAbsolute = join(SUPERSET_ROOT, packageRootDir);
   const tsConfig = getTsConfig(packageRootDirAbsolute);
-  const command = `--noEmit --allowJs --composite false --project ${tsConfig} 
${argsStr} ${declarationFilesStr}`;
 
+  // Process files in batches to avoid command line length limits
+  const batches = [];
+  for (let i = 0; i < tsFiles.length; i += BATCH_SIZE) {
+    batches.push(tsFiles.slice(i, i + BATCH_SIZE));
+  }
+
+  let hasErrors = false;
+
+  for (const [batchIndex, batch] of batches.entries()) {
+    if (batches.length > 1) {
+      console.log(`\nProcessing batch ${batchIndex + 1}/${batches.length} 
(${batch.length} files)...`);
+    }
+
+    const argsStr = batch.join(" ");
+    const declarationFilesStr = declarationFiles.join(" ");

Review Comment:
   ### Inefficient String Concatenation <sub>![category 
Performance](https://img.shields.io/badge/Performance-4f46e5)</sub>
   
   <details>
     <summary>Tell me more</summary>
   
   ###### What is the issue?
   Repeated string concatenation of potentially large file lists for each batch 
creates unnecessary memory allocations.
   
   
   ###### Why this matters
   Large string concatenations can cause memory pressure and garbage collection 
overhead, especially when processing many files.
   
   ###### Suggested change ∙ *Feature Preview*
   Use arrays and spread operator to pass files to the TypeScript compiler:
   ```javascript
   const allFiles = [...batch, ...declarationFiles];
   const command = ['--noEmit', '--allowJs', '--composite', 'false', 
'--project', tsConfig, ...allFiles].join(' ');
   ```
   
   
   ###### Provide feedback to improve future suggestions
   [![Nice 
Catch](https://img.shields.io/badge/👍%20Nice%20Catch-71BC78)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/06b90d6c-18bc-4849-a294-11dc2b410d3a/upvote)
 
[![Incorrect](https://img.shields.io/badge/👎%20Incorrect-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/06b90d6c-18bc-4849-a294-11dc2b410d3a?what_not_true=true)
  [![Not in 
Scope](https://img.shields.io/badge/👎%20Out%20of%20PR%20scope-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/06b90d6c-18bc-4849-a294-11dc2b410d3a?what_out_of_scope=true)
 [![Not in coding 
standard](https://img.shields.io/badge/👎%20Not%20in%20our%20standards-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/06b90d6c-18bc-4849-a294-11dc2b410d3a?what_not_in_standard=true)
 
[![Other](https://img.shields.io/badge/👎%20Other-white)](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/06b90d6c-18bc-4849-a294-11dc2b410d3a)
   </details>
   
   <sub>
   
   💬 Looking for more details? Reply to this comment to chat with Korbit.
   </sub>
   
   <!--- korbi internal id:2b7a457a-1b0d-435c-8718-190ee90bc7fc -->
   
   
   [](2b7a457a-1b0d-435c-8718-190ee90bc7fc)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to