korbit-ai[bot] commented on code in PR #32261: URL: https://github.com/apache/superset/pull/32261#discussion_r1955894779
########## scripts/check-type.js: ########## @@ -0,0 +1,176 @@ +#!/usr/bin/env node + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// @ts-check +const { exit } = require("node:process"); +const { join, dirname } = require("node:path"); +const { readdirSync } = require("node:fs"); +const { chdir, cwd } = require("node:process"); +const { createRequire } = require("node:module"); + +const SUPERSET_ROOT = dirname(__dirname); +const PACKAGE_ARG_REGEX = /^package=/; +const EXCLUDE_DECLARATION_DIR_REGEX = /^excludeDeclarationDir=/; +const DECLARATION_FILE_REGEX = /\.d\.ts$/; + +void (async () => { + const args = process.argv.slice(2); + const { + matchedArgs: [packageArg, excludeDeclarationDirArg], + remainingArgs, + } = extractArgs(args, [PACKAGE_ARG_REGEX, EXCLUDE_DECLARATION_DIR_REGEX]); + + if (!packageArg) { + console.error("package is not specified"); + exit(1); + } + + const packageRootDir = getPackage(packageArg); + const packagePathRegex = new RegExp(`^${packageRootDir}\/`); + const updatedArgs = removePackageSegment(remainingArgs, packagePathRegex); + + const excludedDeclarationDirs = getExcludedDeclarationDirs( + excludeDeclarationDirArg + ); + let declarationFiles = getFilesRecursivelySync( + packageRootDir, + DECLARATION_FILE_REGEX, + excludedDeclarationDirs + ); + declarationFiles = removePackageSegment(declarationFiles, packagePathRegex); + + try { + chdir(join(SUPERSET_ROOT, packageRootDir)); + const packageRequire = createRequire(join(cwd(), "node_modules")); + // Please ensure that tscw-config is installed in the package being type-checked. + const tscw = packageRequire("tscw-config"); + const tsConfig = join(cwd(), "tsconfig.json"); + + const child = + await tscw`--noEmit --allowJs --project ${tsConfig} --composite false ${updatedArgs.join( + " " + )} ${declarationFiles.join(" ")}`; + + if (child.stdout) { + console.log(child.stdout); + } else { + console.log(child.stderr); + } Review Comment: ### Incorrect Log Levels for stdout/stderr <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The script uses console.log for both stdout and stderr output, making it difficult to distinguish between normal output and error conditions in logging systems. ###### Why this matters Log aggregation and monitoring systems often rely on log levels to properly categorize and alert on issues. Using the same log level for both success and error cases can mask problems in production. ###### Suggested change ∙ *Feature Preview* ```javascript if (child.stdout) { console.log(child.stdout); } else { console.error(child.stderr); } ``` </details> <sub> [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/33a45946-2a10-44e4-b14f-b8dd83cab419?suggestedFixEnabled=true) 💬 Chat with Korbit by mentioning @korbit-ai. </sub> <!--- korbi internal id:69025161-10f1-440c-831e-0ea870c1ceba --> ########## scripts/check-type.js: ########## @@ -0,0 +1,176 @@ +#!/usr/bin/env node + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// @ts-check +const { exit } = require("node:process"); +const { join, dirname } = require("node:path"); +const { readdirSync } = require("node:fs"); +const { chdir, cwd } = require("node:process"); +const { createRequire } = require("node:module"); + +const SUPERSET_ROOT = dirname(__dirname); +const PACKAGE_ARG_REGEX = /^package=/; +const EXCLUDE_DECLARATION_DIR_REGEX = /^excludeDeclarationDir=/; +const DECLARATION_FILE_REGEX = /\.d\.ts$/; + +void (async () => { + const args = process.argv.slice(2); + const { + matchedArgs: [packageArg, excludeDeclarationDirArg], + remainingArgs, + } = extractArgs(args, [PACKAGE_ARG_REGEX, EXCLUDE_DECLARATION_DIR_REGEX]); + + if (!packageArg) { + console.error("package is not specified"); + exit(1); + } + + const packageRootDir = getPackage(packageArg); + const packagePathRegex = new RegExp(`^${packageRootDir}\/`); + const updatedArgs = removePackageSegment(remainingArgs, packagePathRegex); + + const excludedDeclarationDirs = getExcludedDeclarationDirs( + excludeDeclarationDirArg + ); + let declarationFiles = getFilesRecursivelySync( + packageRootDir, + DECLARATION_FILE_REGEX, + excludedDeclarationDirs + ); + declarationFiles = removePackageSegment(declarationFiles, packagePathRegex); + + try { + chdir(join(SUPERSET_ROOT, packageRootDir)); + const packageRequire = createRequire(join(cwd(), "node_modules")); + // Please ensure that tscw-config is installed in the package being type-checked. + const tscw = packageRequire("tscw-config"); + const tsConfig = join(cwd(), "tsconfig.json"); + + const child = + await tscw`--noEmit --allowJs --project ${tsConfig} --composite false ${updatedArgs.join( + " " + )} ${declarationFiles.join(" ")}`; + + if (child.stdout) { + console.log(child.stdout); + } else { + console.log(child.stderr); + } + + exit(child.exitCode); + } catch (e) { + console.error(e); + exit(1); + } Review Comment: ### Missing Error Context In Catch Block <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The catch block logs the error without any contextual information about where or why the error occurred. ###### Why this matters When debugging issues in production, simply logging the error object without context makes it difficult to identify the root cause and circumstances of the failure. ###### Suggested change ∙ *Feature Preview* Add context to the error logging to help with debugging: ```javascript catch (e) { console.error('Failed to execute type checking:', e); console.error('Package:', packageRootDir); console.error('Command:', `tscw ${updatedArgs.join(' ')} ${declarationFiles.join(' ')}`); exit(1); } ``` </details> <sub> [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fde22202-8456-494a-8ecb-6644c72dc376?suggestedFixEnabled=true) 💬 Chat with Korbit by mentioning @korbit-ai. </sub> <!--- korbi internal id:b4c62bad-3830-4d82-bd8e-1975dffdb0ef --> ########## scripts/check-type.js: ########## @@ -0,0 +1,176 @@ +#!/usr/bin/env node + +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +// @ts-check +const { exit } = require("node:process"); +const { join, dirname } = require("node:path"); +const { readdirSync } = require("node:fs"); +const { chdir, cwd } = require("node:process"); +const { createRequire } = require("node:module"); + +const SUPERSET_ROOT = dirname(__dirname); +const PACKAGE_ARG_REGEX = /^package=/; +const EXCLUDE_DECLARATION_DIR_REGEX = /^excludeDeclarationDir=/; +const DECLARATION_FILE_REGEX = /\.d\.ts$/; + +void (async () => { + const args = process.argv.slice(2); + const { + matchedArgs: [packageArg, excludeDeclarationDirArg], + remainingArgs, + } = extractArgs(args, [PACKAGE_ARG_REGEX, EXCLUDE_DECLARATION_DIR_REGEX]); + + if (!packageArg) { + console.error("package is not specified"); + exit(1); + } + + const packageRootDir = getPackage(packageArg); + const packagePathRegex = new RegExp(`^${packageRootDir}\/`); + const updatedArgs = removePackageSegment(remainingArgs, packagePathRegex); + + const excludedDeclarationDirs = getExcludedDeclarationDirs( + excludeDeclarationDirArg + ); + let declarationFiles = getFilesRecursivelySync( + packageRootDir, + DECLARATION_FILE_REGEX, + excludedDeclarationDirs + ); Review Comment: ### Blocking File System Operations <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Synchronous file system operations are used to recursively scan directories, which can block the event loop for large directory structures. ###### Why this matters In Node.js, synchronous I/O operations can significantly impact performance by blocking the event loop, especially when dealing with large directory structures or running on systems with slow I/O. ###### Suggested change ∙ *Feature Preview* Replace synchronous operations with asynchronous alternatives using `readdir` with Promise API or callbacks. Here's a high-level approach: ```javascript async function getFilesRecursively(dir, regex, excludedDirs) { const files = await readdir(dir, { withFileTypes: true }); const results = await Promise.all( files.map(async file => { // ... rest of the logic with async operations }) ); return results.flat(); } ``` </details> <sub> [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/443aaed5-c34a-448d-b92d-8acedbe0b00e?suggestedFixEnabled=true) 💬 Chat with Korbit by mentioning @korbit-ai. </sub> <!--- korbi internal id:036f818a-5c1e-49e3-ab48-37d20bb81bc7 --> -- 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]
