refactor: migrate to TypeScript and update dependencies to latest ver…#13
refactor: migrate to TypeScript and update dependencies to latest ver…#13gxcsoccer wants to merge 2 commits into
Conversation
…sions Major changes: - Migrate entire codebase from JavaScript to TypeScript - Update all dependencies to latest stable versions: - commander: 2.19.0 → 12.1.0 - globby: 8.0.1 → 13.2.2 - urllib: 2.31.1 → 4.5.1 - debug: 4.1.0 → 4.3.7 - nunjucks: 3.1.3 → 3.2.4 - eslint: 5.5.0 → 9.17.0 - Remove mkdirp dependency, use Node.js built-in fs.mkdir with recursive option - Add modern tooling: - TypeScript 5.7 with strict type checking - Prettier 3.4 for code formatting - ESLint 9 with flat config - Add comprehensive type definitions for all modules - Update Java build configuration to use Java 8 - Improve code structure and error handling - Maintain backward compatibility with v1.x API Breaking changes: - Minimum Node.js version: 18.0.0 - Package exports now point to dist/ directory 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @gxcsoccer, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request undertakes a significant refactoring of the project, primarily by migrating the entire codebase from JavaScript to TypeScript. This transition aims to enhance type safety, improve code quality, and provide a more robust development experience. Alongside this, all project dependencies have been thoroughly updated to their latest stable versions, and the development toolchain has been modernized with new linters and formatters. These comprehensive changes are designed to boost the project's stability and maintainability, ensuring it remains compatible with current ecosystem standards, all while preserving backward API compatibility. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 migrates the codebase from JavaScript to TypeScript and updates dependencies to the latest versions. It also includes changes to the build process and adds new tooling. The code changes look good overall, but there are a few areas that could be improved to enhance maintainability and readability.
| if (!interfaceName || interfaceName.indexOf('.') === -1) { | ||
| throw new Error( | ||
| `Please config interface name [${facadeName}:${interfaceName}] at proxy.js with right package name` | ||
| ); |
There was a problem hiding this comment.
Throwing an error here will stop the entire process. It might be better to log a warning and continue with the other interfaces. This would prevent a single misconfigured interface from blocking the entire proxy generation process.
Consider logging a warning and continuing instead of throwing an error.
| if (!fs.existsSync(toolsPath)) { | ||
| throw new Error( | ||
| 'lib/tools.jar not found in JAVA_HOME! Only support jdk1.8, javahome: ' + javahome | ||
| ); | ||
| } |
There was a problem hiding this comment.
This error message is very specific to JDK 1.8. Consider making it more generic or checking for other common issues that might prevent tools.jar from being found. A more generic message would be useful for users who may not be familiar with the specific requirements of this tool.
| if (!Array.isArray(appConfig.services)) { | ||
| throw new Error("proxyConfig.services can't be empty."); |
There was a problem hiding this comment.
This check ensures that proxyConfig.services is an array. If it's not, an error is thrown. However, it might be more robust to handle cases where proxyConfig.services is undefined or null. Consider adding a check for null or undefined to prevent unexpected errors.
| if (!sources) { | ||
| throw new Error( | ||
| `${key}-sources.jar is missing, Please contact the package administrator.` | ||
| ); |
There was a problem hiding this comment.
This error message is very specific. Consider providing more context or guidance on how to resolve the missing sources. A more helpful message would guide the user on how to resolve the issue, such as contacting the package administrator or configuring the build to include sources.
| "typecheck": "tsc --noEmit", | ||
| "lint": "eslint .", | ||
| "cov": "npm run build && egg-bin cov", | ||
| "ci": "npm run lint && npm run cov", |
There was a problem hiding this comment.
Consider adding a postbuild script to run build:java to ensure that the Java code is built after the TypeScript code. This would ensure that the Java code is always up-to-date after a build.
"prepublishOnly": "npm run build",
"postbuild": "npm run build:java"
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughMigrates project to TypeScript with version 2.0.0, introduces new build tooling (ESLint 9, Prettier, TypeScript 5.7), adds core library code for Java JAR-to-proxy generation with Maven dependency management, establishes a CLI entry point, and updates project configuration with TypeScript declaration files and strict compiler settings. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI<br/>(jar2proxy.ts)
participant J2P as Jar2proxy<br/>orchestrator
participant Cfg as ProxyConfig<br/>loader
participant Dep as Dependency<br/>manager
participant JP as JarProcessor
participant Nj as Nunjucks<br/>renderer
CLI->>J2P: new Jar2proxy(options)
CLI->>J2P: run()
J2P->>Cfg: readConfig()
Cfg-->>J2P: ProxyConfig
J2P->>Dep: download()
Dep-->>J2P: JARs downloaded
J2P->>JP: genAST(proxyConfig)
JP-->>J2P: proxy-ast.json
rect rgba(100, 200, 150, 0.3)
Note over J2P: AST loaded &<br/>validated
end
J2P->>J2P: renderByAST()
J2P->>Nj: render templates
Nj-->>J2P: proxy files
J2P-->>CLI: success + log path
CLI->>CLI: exit(0)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Justification: Substantial TypeScript migration with ~20 new interdependent modules across Java integration, dependency resolution, templating, and CLI layers. Logic is dense (AST parsing orchestration, Maven SNAPSHOT handling, plugin config merging with conflict detection), spread across heterogeneous file types (type definitions, class implementations, platform-specific logic). While some files are homogeneous type stubs, the core logic modules require careful reasoning about data flow, error paths, and external tool integration. Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
没想到还能更新，node 版本直接升级到 20 吧，18 已经不是 LTS 了 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
astparser/build.xml (1)
10-12: Upgrade severely outdated Java dependencies to resolve critical security vulnerabilities.The dependencies in lines 10–12 contain known critical vulnerabilities:
- log4j-core-2.11.1: Vulnerable to remote code injection (Log4Shell, CVE-2021-44228 and related). Upgrade to 2.12.2 or later.
- fastjson-1.2.48: Vulnerable to unsafe deserialization (HIGH severity). Upgrade to 1.2.83 or later.
These must be updated immediately to mitigate security risks in the project.
package.json (1)
25-38: Duplicate"build"script confirmed; TypeScript compilation is being skipped.The duplicate keys (lines 27 and 35) cause only the last one to take effect—
"build": "node bin/build.js"which runs ant exclusively. This meanstscnever executes duringnpm run build, breaking the TS compilation path for tests and publish workflows.Your proposed solution is correct: consolidate to
build:ts, usebuildas a wrapper, and leverage thepostbuildhook to auto-runbuild:java. This ensures both TypeScript and Java builds execute in the proper sequence.The changes can be applied as proposed in the diff.
♻️ Duplicate comments (2)
lib/java/javahome.ts (1)
22-25: Harden macOS resolution and improve error message.Guard
java_homecall and provide an actionable, version-agnostic error.- if (!javahome) { - javahome = exec('/usr/libexec/java_home'); - debug('osx.libexec', javahome); - } + if (!javahome) { + try { + javahome = exec('/usr/libexec/java_home'); + debug('osx.libexec', javahome); + } catch (err) { + debug('osx.libexec.error', err); + } + } @@ - if (!fs.existsSync(toolsPath)) { - throw new Error( - 'lib/tools.jar not found in JAVA_HOME! Only support jdk1.8, javahome: ' + javahome - ); - } + if (!fs.existsSync(toolsPath)) { + throw new Error( + `lib/tools.jar not found under JAVA_HOME (${javahome}). This tool requires JDK 8 (tools.jar present).` + ); + }As per previous review comments.
Also applies to: 38-41
lib/java/jar.ts (1)
85-92: Optionally continue on bad interface entries instead of throwing.To avoid whole-run aborts, log and skip invalid entries.
- if (!interfaceName || interfaceName.indexOf('.') === -1) { - throw new Error( - `Please config interface name [${facadeName}:${interfaceName}] at proxy.js with right package name` - ); - } + if (!interfaceName || interfaceName.indexOf('.') === -1) { + this.logger.warn?.( + 'Invalid interface name [%s:%s]; skipping. Expected fully-qualified name.', + facadeName, + interfaceName + ); + continue; + }
🧹 Nitpick comments (13)
lib/types/cassandra-map.d.ts (1)
1-3: Consider usingunknowninstead ofanyfor better type safety.The
anytype bypasses TypeScript's type checking. Since this is a stringify function that likely accepts any object,unknownwould provide better type safety while remaining flexible.Apply this diff to improve type safety:
declare module 'cassandra-map' { - export function stringify(obj: any): string; + export function stringify(obj: unknown): string; }lib/types/xml2map.d.ts (1)
1-8: Consider usingunknownfor the return type to improve type safety.The
anyreturn type bypasses TypeScript's type checking. Usingunknownwould require consumers to narrow the type before use, improving type safety while remaining flexible for JSON parsing results.Apply this diff:
declare module 'xml2map' { interface Xml2Map { - tojson: (xml: string) => any; + tojson: (xml: string) => unknown; } const xml2map: Xml2Map; export = xml2map; }bin/jar2proxy.ts (1)
39-41: Remove arbitrary delay before process exit.The 1-second
setTimeoutbeforeprocess.exit()appears unnecessary and slows down CLI execution. If the delay is intended to allow async operations to complete, consider using proper async cleanup or flushing the logger instead.Consider one of these approaches:
Option 1: Remove the delay entirely (if no cleanup is needed):
- setTimeout(() => { - process.exit(); - }, 1000); + process.exit(0);Option 2: Flush the logger explicitly (if waiting for logs):
+ if (jar2proxy['logger']?.flush) { + jar2proxy['logger'].flush(); + } - setTimeout(() => { - process.exit(); - }, 1000); + process.exit(0);lib/nunjucks.ts (3)
11-16: Normalize fully-qualified type names before switching.Switching on raw
strmisses cases likejava.lang.String. Normalize once withsplitLast(str, '.')and use that in the switch.-function getType(str: string, field?: Field, onlyBasis?: boolean): string { - const generic = field?.generic; - - switch (str) { +function getType(str: string, field?: Field, onlyBasis?: boolean): string { + const generic = field?.generic; + const base = splitLast(str, '.'); + + switch (base) {Also applies to: 38-66
120-127:firstParammay returnundefinedbut is typed asstring.Adjust the return type or default to
''to avoid runtime surprises.- firstParam(str: string): string { + firstParam(str: string): string | undefined { return (str || '') .trim() .split(',') .filter((s) => !!s.trim()) .map((s) => s.trim())[0]; },
140-146: Guard for empty strings inupperFirst/lowerFirst.Avoid
str[0]on empty input.- upperFirst(str: string): string { - return str[0].toUpperCase() + str.substring(1); - }, + upperFirst(str: string): string { + return str ? str[0].toUpperCase() + str.substring(1) : str; + }, - lowerFirst(str: string): string { - return str[0].toLowerCase() + str.substring(1); - }, + lowerFirst(str: string): string { + return str ? str[0].toLowerCase() + str.substring(1) : str; + },package.json (1)
17-21: Consider dropping"lib"fromfilesto publish only compiled artifacts.Not required, but reduces package size and prevents consumers from importing sources by mistake.
"files": [ - "bin", - "lib", - "dist" + "dist" ],lib/java/javahome.ts (2)
14-16: Usetrim()to normalize output fromexec.
replace('\n','')removes only one newline;trim()is safer.function exec(cmd: string): string { - return (execSync(cmd) || '').toString().replace('\n', ''); + return (execSync(cmd) || '').toString().trim(); }
107-111: Clarify final error and include platform hint.Minor, but helps users fix env quickly.
- throw new Error( - (javaHomeConfig?.toolsPath || 'JAVA_HOME') + " Can't be found or not correct!" - ); + throw new Error( + `${javaHomeConfig?.toolsPath || 'JAVA_HOME'} cannot be found or is invalid on platform "${platform}".` + );lib/proxy_config.ts (1)
109-111: The array check is adequate but the error message could be clearer.In response to the previous review comment: The check
!Array.isArray(appConfig.services)correctly handlesnullandundefinedcases sinceArray.isArray()returnsfalsefor both. However, the error message "proxyConfig.services can't be empty" is misleading—it's actually checking that services is an array, not that it's non-empty.Consider clarifying the error message:
if (!Array.isArray(appConfig.services)) { - throw new Error("proxyConfig.services can't be empty."); + throw new Error("proxyConfig.services must be an array."); }lib/jar2proxy.ts (1)
130-134: Validate AST structure after loading.Line 134 uses
require(astfile)which could succeed even if the file contains invalid or incomplete data. Consider validating that the loaded object has the expected structure (proxyMap, classMap, enumMap).if (!fs.existsSync(astfile)) { throw new Error("Ast result file can't be found"); } - this.astjson = require(astfile); + const astjson = require(astfile); + if (!astjson || !astjson.proxyMap || !astjson.classMap || !astjson.enumMap) { + throw new Error('Invalid AST file structure'); + } + this.astjson = astjson;lib/maven/dependency.ts (2)
100-103: Enhance error message with actionable guidance.The error message for missing sources could provide more helpful guidance to users, such as mentioning the
ignoreSourcesconfiguration option.if (!sources) { throw new Error( - `${key}-sources.jar is missing, Please contact the package administrator.` + `${key}-sources.jar is missing. Please contact the package administrator or set "ignoreSources: true" in the dependency configuration to skip downloading sources.` ); }
194-202: Add retry delay and use logger consistently.The retry logic immediately retries failed requests without any delay, which could overwhelm failing servers. Additionally, lines 195 and 201 use
console.warnandconsole.errorinstead of the logger.if (err.code === 'ENOTFOUND' || err.code === 'ECONNRESET' || err.code === 'ENETRESET') { - console.warn( + this.logger.warn?.( '[jar2proxy] request %s error: %s, retry after 100ms, left: %s', url, err, retry ); - console.error(err); + this.logger.error(err); + // Add delay before retry + await new Promise(resolve => setTimeout(resolve, 100)); return await this.request(url, args, retry); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
.eslintignore(1 hunks).gitignore(1 hunks).prettierignore(1 hunks).prettierrc(1 hunks)UPGRADE.md(1 hunks)astparser/build.xml(1 hunks)bin/jar2proxy.ts(1 hunks)eslint.config.js(1 hunks)lib/jar2proxy.ts(1 hunks)lib/java/jar.ts(1 hunks)lib/java/javahome.ts(1 hunks)lib/java/platform.ts(1 hunks)lib/logger.ts(1 hunks)lib/maven/dependency.ts(1 hunks)lib/nunjucks.ts(1 hunks)lib/proxy_config.ts(1 hunks)lib/types.ts(1 hunks)lib/types/cassandra-map.d.ts(1 hunks)lib/types/copy-to.d.ts(1 hunks)lib/types/egg-utils.d.ts(1 hunks)lib/types/mini-logger.d.ts(1 hunks)lib/types/xml2map.d.ts(1 hunks)package.json(2 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
lib/nunjucks.ts (1)
lib/nunjucks.js (6)
generic(7-7)key(37-37)items(69-69)filters(72-144)match(93-93)type(135-135)
lib/types/xml2map.d.ts (1)
lib/maven/dependency.js (1)
xml2map(8-8)
lib/java/jar.ts (1)
lib/types.ts (3)
Logger(50-55)JarProcessorOptions(103-105)ProxyConfig(1-10)
lib/java/javahome.ts (1)
lib/java/javahome.js (4)
roots(63-63)root(65-65)files(67-67)file(69-69)
lib/types/mini-logger.d.ts (1)
lib/types.ts (1)
Logger(50-55)
lib/maven/dependency.ts (1)
lib/types.ts (3)
Dependency(29-34)DependencyConfig(94-101)Logger(50-55)
lib/types/cassandra-map.d.ts (1)
lib/nunjucks.js (1)
stringify(4-4)
bin/jar2proxy.ts (2)
bin/jar2proxy.js (3)
program(5-5)jar2proxy(23-23)Jar2proxy(6-6)test/global.js (1)
process(6-6)
lib/jar2proxy.ts (3)
bin/jar2proxy.js (1)
Jar2proxy(6-6)lib/types.ts (7)
Jar2proxyConfig(43-48)Logger(50-55)ASTJson(57-61)Jar2proxyOptions(36-41)ProxyConfig(1-10)Dependency(29-34)ApiConfig(20-27)lib/logger.ts (1)
createLogger(11-23)
lib/proxy_config.ts (2)
lib/types.ts (4)
Logger(50-55)Dependency(29-34)Service(12-18)ProxyConfig(1-10)lib/proxy_config.js (1)
getPlugins(6-6)
lib/types/egg-utils.d.ts (1)
lib/proxy_config.js (1)
getPlugins(6-6)
lib/logger.ts (1)
lib/types.ts (1)
Logger(50-55)
🪛 Biome (2.1.2)
package.json
[error] 27-27: The key build was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Socket Security: Project Report
🔇 Additional comments (16)
astparser/build.xml (1)
25-25: Java 8 upgrade looks good.The update from Java 1.6 to 1.8 aligns well with the overall modernization effort in this PR.
.prettierignore (1)
1-7: LGTM! Standard ignore patterns.The ignore patterns appropriately exclude build artifacts, dependencies, and generated directories from Prettier formatting.
.gitignore (1)
23-25: LGTM! Appropriate TypeScript build output patterns.The added ignore patterns correctly exclude TypeScript compilation artifacts (dist/ and *.tsbuildinfo), aligning with the migration to TypeScript and dist-based build structure.
.eslintignore (1)
4-6: LGTM! Appropriate ignore patterns for build artifacts.The patterns correctly exclude TypeScript build outputs (dist/), Java parser artifacts (astparser/), and binary libraries (bin/libs/).
.prettierrc (1)
1-10: LGTM! Standard Prettier configuration.The configuration uses sensible defaults appropriate for a TypeScript project, including consistent spacing, quote style, and line endings.
lib/types/copy-to.d.ts (1)
1-9: LGTM! Well-typed fluent API.The type signature correctly models the fluent chaining pattern (
copy(obj).and(target).to(extra)) with proper generic constraints and intersection types to represent the merged result.UPGRADE.md (1)
1-111: LGTM! Comprehensive upgrade documentation.The upgrade guide is thorough and well-organized, covering all major changes, migration steps, and known issues effectively.
lib/types/egg-utils.d.ts (1)
1-16: LGTM! Type declarations are accurate.The type definitions correctly model the egg-utils API and align with the usage in lib/proxy_config.js.
eslint.config.js (1)
1-60: LGTM! Well-structured ESLint 9 flat config.The configuration is comprehensive with appropriate rules for a TypeScript Node.js project. The
no-console: 'off'rule is suitable for a CLI tool that requires console output.tsconfig.json (1)
1-34: LGTM! Robust TypeScript configuration.The TypeScript config is well-configured with strict type checking, appropriate output settings, and correct include/exclude patterns for a Node.js library.
lib/java/platform.ts (1)
1-22: LGTM! Correct platform detection logic.The platform mapping correctly handles all common operating systems and maps them to the appropriate simplified platform identifiers.
lib/logger.ts (1)
1-23: LGTM! Logger configuration is correct.The logger setup is appropriate, using the native
fs.mkdirSyncwith recursive option. Theseperatorspelling on line 21 matches the mini-logger library's API (which has the same spelling in its interface).lib/types/mini-logger.d.ts (1)
1-21: LGTM! Type declarations match the external library API.The type definitions correctly model the mini-logger module's interface, including the
seperatorspelling which matches the external library's actual API.lib/types.ts (1)
1-105: Type shapes look solid and align with usage.Interfaces are clear and cohesive; no blockers.
lib/proxy_config.ts (2)
51-74: Consider wrapping the dynamic require in error handling.Line 52 uses
require(this.proxyConfigPath)without a try-catch block. If the config file contains syntax errors or cannot be loaded, this will throw an unhandled exception. WhilegetPluginsis properly wrapped in error handling (lines 58-67), the initial config load is not.Consider applying this pattern:
readConfig(): ProxyConfigType { - const proxyConfig = require(this.proxyConfigPath) as ProxyConfigWithPlugin; + let proxyConfig: ProxyConfigWithPlugin; + try { + proxyConfig = require(this.proxyConfigPath) as ProxyConfigWithPlugin; + } catch (err) { + this.logger.error('Failed to load proxy config from %s', this.proxyConfigPath); + this.logger.error(err); + throw new Error(`Failed to load proxy config: ${err}`); + } const { eggFramework = 'egg' } = proxyConfig;
128-132: Type mismatch: dependency field handling.The type
ServiceWithDependencydeclaresdependency: DependencyWithPlugin[], but this code handles both array and non-array cases (lines 130-132). This suggests either the types are incorrect or the runtime data doesn't match the declared types.Verify whether
dependencycan actually be a single object in the config files, and if so, update the type definition to reflect this:interface ServiceWithDependency extends Service { dependency: DependencyWithPlugin | DependencyWithPlugin[]; }Or ensure all configs provide arrays and remove the runtime check.
| dist/ | ||
| astparser/ | ||
| bin/libs/ | ||
| **/*.d.ts |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Overly broad pattern ignores hand-written type declarations.
The pattern **/*.d.ts ignores ALL .d.ts files, including the hand-written type declarations in lib/types/*.d.ts that are part of this PR. These custom declarations should be linted for quality and consistency. Generated declaration files in dist/ are already excluded by the dist/ pattern.
Apply this diff to use a more targeted pattern:
-**/*.d.ts
+dist/**/*.d.tsThis change ensures hand-written type declarations are linted while still excluding generated build outputs.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **/*.d.ts | |
| dist/**/*.d.ts |
🤖 Prompt for AI Agents
In .eslintignore around line 7, the pattern `**/*.d.ts` is overly broad and
ignores all declaration files including hand-written ones in lib/types; change
the ignore to target only generated declarations (e.g., replace with
`dist/**/*.d.ts` or add a more specific path to generated outputs) so that
lib/types/*.d.ts remain linted while build artifacts stay excluded.
| console.log( | ||
| '[jar2proxy] You can see detail at %s/logs/jar2proxy-*.log', | ||
| jar2proxy['config'].baseDir | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid bracket notation to access private fields.
Using jar2proxy['config'] with bracket notation bypasses TypeScript's access control. Consider either:
- Making the
configproperty public if external access is needed, or - Passing
baseDirthrough the options object and using it directly here
Apply this diff to use the local opts variable instead:
console.log(
- '[jar2proxy] You can see detail at %s/logs/jar2proxy-*.log',
- jar2proxy['config'].baseDir
+ '[jar2proxy] You can see detail at %s/logs/jar2proxy-*.log',
+ opts.baseDir
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| '[jar2proxy] You can see detail at %s/logs/jar2proxy-*.log', | |
| jar2proxy['config'].baseDir | |
| ); | |
| console.log( | |
| '[jar2proxy] You can see detail at %s/logs/jar2proxy-*.log', | |
| opts.baseDir | |
| ); |
🤖 Prompt for AI Agents
In bin/jar2proxy.ts around lines 35-38, the console.log is using bracket
notation to access a private field (jar2proxy['config'].baseDir); replace that
with the local opts variable (e.g., opts.baseDir) so you don't bypass TypeScript
access control. Ensure the opts variable is in scope (or passed into this
module/function) and has a baseDir property; if opts isn't available, pass
baseDir through the options or make the config property intentionally public
before accessing it.
| async run(): Promise<void> { | ||
| try { | ||
| const { baseDir } = this.config; | ||
| const config = this.loadConfig(); | ||
| this.logger.info('%j', config); | ||
| this.proxyConfig = config; | ||
|
|
||
| const depd = new Dependency({ | ||
| baseDir, | ||
| directoryToJar: config.directoryToJar, | ||
| mavenRepository: config.mavenRepository, | ||
| dependencies: config.dependencies, | ||
| logger: this.logger, | ||
| jarDir: this.jarDir, | ||
| }); | ||
|
|
||
| await depd.download(); | ||
| await this.genAST(config); | ||
| await this.renderByAST(); | ||
| } catch (err) { | ||
| this.logger.error(err); | ||
| if (this.logger.flush) { | ||
| this.logger.flush(); | ||
| } | ||
| console.log((err as Error).stack); | ||
| } | ||
| } |
There was a problem hiding this comment.
Error handling masks failures from callers.
The run method catches all errors (line 104) but doesn't re-throw them, causing the Promise to resolve successfully even when the operation fails. This prevents callers (such as the CLI in bin/jar2proxy.ts) from detecting failures and exiting with appropriate error codes.
Additionally, line 109 uses console.log instead of the logger.
Apply this diff:
async run(): Promise<void> {
try {
const { baseDir } = this.config;
const config = this.loadConfig();
this.logger.info('%j', config);
this.proxyConfig = config;
const depd = new Dependency({
baseDir,
directoryToJar: config.directoryToJar,
mavenRepository: config.mavenRepository,
dependencies: config.dependencies,
logger: this.logger,
jarDir: this.jarDir,
});
await depd.download();
await this.genAST(config);
await this.renderByAST();
} catch (err) {
this.logger.error(err);
if (this.logger.flush) {
this.logger.flush();
}
- console.log((err as Error).stack);
+ this.logger.error((err as Error).stack);
+ throw err;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async run(): Promise<void> { | |
| try { | |
| const { baseDir } = this.config; | |
| const config = this.loadConfig(); | |
| this.logger.info('%j', config); | |
| this.proxyConfig = config; | |
| const depd = new Dependency({ | |
| baseDir, | |
| directoryToJar: config.directoryToJar, | |
| mavenRepository: config.mavenRepository, | |
| dependencies: config.dependencies, | |
| logger: this.logger, | |
| jarDir: this.jarDir, | |
| }); | |
| await depd.download(); | |
| await this.genAST(config); | |
| await this.renderByAST(); | |
| } catch (err) { | |
| this.logger.error(err); | |
| if (this.logger.flush) { | |
| this.logger.flush(); | |
| } | |
| console.log((err as Error).stack); | |
| } | |
| } | |
| async run(): Promise<void> { | |
| try { | |
| const { baseDir } = this.config; | |
| const config = this.loadConfig(); | |
| this.logger.info('%j', config); | |
| this.proxyConfig = config; | |
| const depd = new Dependency({ | |
| baseDir, | |
| directoryToJar: config.directoryToJar, | |
| mavenRepository: config.mavenRepository, | |
| dependencies: config.dependencies, | |
| logger: this.logger, | |
| jarDir: this.jarDir, | |
| }); | |
| await depd.download(); | |
| await this.genAST(config); | |
| await this.renderByAST(); | |
| } catch (err) { | |
| this.logger.error(err); | |
| if (this.logger.flush) { | |
| this.logger.flush(); | |
| } | |
| this.logger.error((err as Error).stack); | |
| throw err; | |
| } | |
| } |
🤖 Prompt for AI Agents
In lib/jar2proxy.ts around lines 85 to 111, the catch block currently logs the
error, calls logger.flush, and prints the stack with console.log but does not
re-throw, causing the Promise to resolve on failure; replace console.log with
logger.error (including the error stack/details), call logger.flush if present,
and then re-throw the caught error (or throw a new Error wrapping it) so the
Promise rejects and callers can observe failures and exit with proper codes.
| private renderProxy(): void { | ||
| const defaultOptions: any = { ...DEFAULT_API_OPTIONS }; | ||
| defaultOptions.responseTimeout = | ||
| this.proxyConfig.responseTimeout || defaultOptions.responseTimeout; | ||
| defaultOptions.version = this.proxyConfig.version || defaultOptions.version; | ||
| defaultOptions.group = this.proxyConfig.group || defaultOptions.group; | ||
| defaultOptions.tpl = this.config.defaultTpl || defaultOptions.tpl; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid using any type for defaultOptions.
Line 154 declares defaultOptions as any, which bypasses TypeScript's type checking. The type is already defined as DefaultApiOptions, so use it.
- const defaultOptions: any = { ...DEFAULT_API_OPTIONS };
+ const defaultOptions: DefaultApiOptions = { ...DEFAULT_API_OPTIONS };
defaultOptions.responseTimeout =
this.proxyConfig.responseTimeout || defaultOptions.responseTimeout;🤖 Prompt for AI Agents
In lib/jar2proxy.ts around lines 153 to 160, replace the use of the any type for
defaultOptions with the proper DefaultApiOptions type: declare defaultOptions as
DefaultApiOptions (or as const defaultOptions: DefaultApiOptions = {
...DEFAULT_API_OPTIONS } ), ensure DefaultApiOptions is imported or available in
this file, and if the spread assignment needs narrowing use a safe cast or
create a new object literal typed as DefaultApiOptions; keep the subsequent
property overrides the same but typed so TypeScript enforces correctness.
| extract(jarFiles: string[], distdir: string): void { | ||
| debug('extract javahome: %s', javahome); | ||
| const winprefix = | ||
| platform === 'win' && /^\w:/.test(distdir) ? distdir.substring(0, 2) + ' && ' : ''; | ||
| const binjar = join(javahome, 'bin/jar'); | ||
|
|
||
| for (const jarFile of jarFiles) { | ||
| if (!fs.existsSync(jarFile)) { | ||
| console.log('[jar2proxy:extract] jar file not exists: %s.', jarFile); | ||
| continue; | ||
| } | ||
|
|
||
| // #37 compatible with windows cross drive cd, d:/tmp | ||
| const cmd = `${winprefix} cd ${distdir} && "${binjar}" -xf ${jarFile}`; | ||
| debug('exec: %s', cmd); | ||
| execSync(cmd, { encoding: 'utf8' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Command injection and path issues in extract(). Use spawnSync with cwd instead of shelling out.
Interpolated distdir/jarFile allow injection and break on spaces; also mixes console.log with logger.
extract(jarFiles: string[], distdir: string): void {
debug('extract javahome: %s', javahome);
- const winprefix =
- platform === 'win' && /^\w:/.test(distdir) ? distdir.substring(0, 2) + ' && ' : '';
const binjar = join(javahome, 'bin/jar');
for (const jarFile of jarFiles) {
if (!fs.existsSync(jarFile)) {
- console.log('[jar2proxy:extract] jar file not exists: %s.', jarFile);
+ this.logger.warn?.('[jar2proxy:extract] jar file not exists: %s.', jarFile);
continue;
}
- // #37 compatible with windows cross drive cd, d:/tmp
- const cmd = `${winprefix} cd ${distdir} && "${binjar}" -xf ${jarFile}`;
- debug('exec: %s', cmd);
- execSync(cmd, { encoding: 'utf8' });
+ const res = spawnSync(binjar, ['-xf', jarFile], {
+ cwd: distdir,
+ stdio: 'pipe',
+ encoding: 'utf8',
+ });
+ if (res.stdout) this.logger.info('%s', res.stdout);
+ if (res.stderr) this.logger.info('%s', res.stderr);
+ if (res.status !== 0) {
+ throw new Error(`[jar2proxy:extract] jar failed with code ${res.status} for ${jarFile}`);
+ }
}
}🤖 Prompt for AI Agents
lib/java/jar.ts lines 27-44: the extract() implementation shells out a
constructed command string using interpolated distdir and jarFile which allows
command injection and breaks on paths with spaces; replace the execSync
string-based call with child_process.spawnSync calling the jar binary directly
(use join(javahome, 'bin', 'jar') as the executable and pass ['-xf', jarFile] as
args) and set cwd: distdir to avoid manual cd/win-drive hacks; propagate
stdout/stderr or capture encoding as needed, replace console.log with the module
logger (debug or appropriate logger), and keep the existsSync check and proper
error handling for non-zero exit codes.
| const result = spawnSync(bin, cmd, { | ||
| stdio: 'pipe', | ||
| }); | ||
|
|
||
| this.logger.info('%s', result.stdout); | ||
| this.logger.info('%s', result.stderr); | ||
|
|
||
| return astfile; | ||
| } |
There was a problem hiding this comment.
Check parse() exit code, set encoding, and verify AST file exists.
Currently returns a path even on failure and logs Buffers.
- const result = spawnSync(bin, cmd, {
- stdio: 'pipe',
- });
-
- this.logger.info('%s', result.stdout);
- this.logger.info('%s', result.stderr);
-
- return astfile;
+ const result = spawnSync(bin, cmd, {
+ stdio: 'pipe',
+ encoding: 'utf8',
+ });
+ if (result.stdout) this.logger.info('%s', result.stdout);
+ if (result.stderr) this.logger.info('%s', result.stderr);
+ if (result.status !== 0) {
+ throw new Error(`[jar2proxy:parse] java exited with code ${result.status}`);
+ }
+ if (!fs.existsSync(astfile)) {
+ throw new Error(`[jar2proxy:parse] AST file not created: ${astfile}`);
+ }
+ return astfile;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = spawnSync(bin, cmd, { | |
| stdio: 'pipe', | |
| }); | |
| this.logger.info('%s', result.stdout); | |
| this.logger.info('%s', result.stderr); | |
| return astfile; | |
| } | |
| const result = spawnSync(bin, cmd, { | |
| stdio: 'pipe', | |
| encoding: 'utf8', | |
| }); | |
| if (result.stdout) this.logger.info('%s', result.stdout); | |
| if (result.stderr) this.logger.info('%s', result.stderr); | |
| if (result.status !== 0) { | |
| throw new Error(`[jar2proxy:parse] java exited with code ${result.status}`); | |
| } | |
| if (!fs.existsSync(astfile)) { | |
| throw new Error(`[jar2proxy:parse] AST file not created: ${astfile}`); | |
| } | |
| return astfile; | |
| } |
🤖 Prompt for AI Agents
In lib/java/jar.ts around lines 100 to 108, the parse() function currently calls
spawnSync without an encoding, logs raw Buffers, and returns astfile even on
failure; update the spawnSync call to include an encoding: 'utf8' (or set
result.stdout.toString('utf8') / result.stderr.toString('utf8') if you prefer),
check result.status (and result.error) and if non-zero/errored, log stderr and
throw or return an error instead of returning astfile, then after successful
exit verify the AST file exists (fs.existsSync(astfile)) and only return astfile
if it exists, otherwise log/throw an appropriate error.
| } else { | ||
| const versions = versioning.snapshotVersions.snapshotVersion; | ||
| const sources = versions.filter((v: any) => v.classifier === 'sources')[0]; | ||
|
|
||
| if (!sources) { | ||
| throw new Error( | ||
| `${key}-sources.jar is missing, Please contact the package administrator.` | ||
| ); | ||
| } | ||
|
|
||
| const jar = versions.filter((v: any) => !v.classifier)[0]; | ||
| jarUrls[key + '.jar'] = copy({ jarVersion: sources.value }).and(helper).to(); | ||
| if (!helper.ignoreSources) { | ||
| jarUrls[key + '-sources.jar'] = copy({ jarVersion: `${jar.value}-sources` }) | ||
| .and(helper) | ||
| .to({ sources: true }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: JAR and sources versions are swapped.
Lines 107 and 109 have the jar and sources versions reversed:
- Line 107 assigns
sources.valueto the main jar URL - Line 109 assigns
jar.valueto the sources jar URL
This will cause downloads to fail or retrieve incorrect artifacts.
Apply this fix:
const sources = versions.filter((v: any) => v.classifier === 'sources')[0];
if (!sources) {
throw new Error(
`${key}-sources.jar is missing, Please contact the package administrator.`
);
}
const jar = versions.filter((v: any) => !v.classifier)[0];
- jarUrls[key + '.jar'] = copy({ jarVersion: sources.value }).and(helper).to();
+ jarUrls[key + '.jar'] = copy({ jarVersion: jar.value }).and(helper).to();
if (!helper.ignoreSources) {
- jarUrls[key + '-sources.jar'] = copy({ jarVersion: `${jar.value}-sources` })
+ jarUrls[key + '-sources.jar'] = copy({ jarVersion: sources.value })
.and(helper)
.to({ sources: true });
}🤖 Prompt for AI Agents
In lib/maven/dependency.ts around lines 96 to 113, the jar and sources versions
are swapped: the main jar URL is built from sources.value and the sources jar
URL is built from jar.value; fix by assigning jarUrls[key + '.jar'] to use
jar.value as the jarVersion and, when creating the sources entry (if
!helper.ignoreSources), assign jarUrls[key + '-sources.jar'] to use
sources.value (and keep the existing to({ sources: true }) behavior); ensure the
keys remain exactly key + '.jar' and key + '-sources.jar' and no other logic is
changed.
| antx(val: string): string { | ||
| val = val || ''; | ||
| const match = val.match(/^\${(.*)}$/); | ||
| if (match && match[1]) { | ||
| return "app.config['" + match[1] + "']"; | ||
| } | ||
| return "'" + val + "'"; | ||
| }, |
There was a problem hiding this comment.
Escape values in antx to avoid code injection in generated output.
Current implementation allows quotes and arbitrary content; prefer strict matching and JSON-escaped output.
- antx(val: string): string {
- val = val || '';
- const match = val.match(/^\${(.*)}$/);
- if (match && match[1]) {
- return "app.config['" + match[1] + "']";
- }
- return "'" + val + "'";
- },
+ antx(val: string): string {
+ const s = String(val ?? '');
+ const match = s.match(/^\${([\w.]+)}$/);
+ if (match && match[1]) {
+ return `app.config[${JSON.stringify(match[1])}]`;
+ }
+ return JSON.stringify(s);
+ },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| antx(val: string): string { | |
| val = val || ''; | |
| const match = val.match(/^\${(.*)}$/); | |
| if (match && match[1]) { | |
| return "app.config['" + match[1] + "']"; | |
| } | |
| return "'" + val + "'"; | |
| }, | |
| antx(val: string): string { | |
| const s = String(val ?? ''); | |
| const match = s.match(/^\${([\w.]+)}$/); | |
| if (match && match[1]) { | |
| return `app.config[${JSON.stringify(match[1])}]`; | |
| } | |
| return JSON.stringify(s); | |
| }, |
🤖 Prompt for AI Agents
In lib/nunjucks.ts around lines 102 to 109, the antx helper currently allows
unescaped values and returns raw concatenated strings which can lead to code
injection; tighten the key matching to only accept a safe identifier pattern
(e.g. /^[A-Za-z0-9_.]+$/) when extracting match[1] and, for both the matched key
and the fallback string value, emit JSON-escaped output (use
JSON.stringify(match[1]) when embedding into app.config[...] or
JSON.stringify(val) for the literal) so all quotes and special characters are
safely escaped in the generated code.
| console.log( | ||
| '[jar2proxy] `%s` not found in config/proxy.js use plugin config first [%s]', | ||
| keyName, | ||
| (mergedConfig as any)[keyName] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use logger instead of console.log for consistency.
This code uses console.log directly instead of this.logger.info, which is inconsistent with the rest of the codebase. The logger provides better control over log output and formatting.
- console.log(
- '[jar2proxy] `%s` not found in config/proxy.js use plugin config first [%s]',
- keyName,
- (mergedConfig as any)[keyName]
- );
+ this.logger.info(
+ '[jar2proxy] `%s` not found in config/proxy.js use plugin config first [%s]',
+ keyName,
+ (mergedConfig as any)[keyName]
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log( | |
| '[jar2proxy] `%s` not found in config/proxy.js use plugin config first [%s]', | |
| keyName, | |
| (mergedConfig as any)[keyName] | |
| ); | |
| this.logger.info( | |
| '[jar2proxy] `%s` not found in config/proxy.js use plugin config first [%s]', | |
| keyName, | |
| (mergedConfig as any)[keyName] | |
| ); |
🤖 Prompt for AI Agents
In lib/proxy_config.ts around lines 171 to 175, replace the direct console.log
call with the class instance logger to maintain consistency; use
this.logger.info with the same formatted message and arguments (keyName and
(mergedConfig as any)[keyName]) and ensure the surrounding method has access to
this.logger (or obtain the correct logger instance) so TypeScript typing and
linting remain satisfied.
|
|
||
| if (versions.length === 1) { | ||
| // print the same depd, but do not stop task | ||
| console.warn(message); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use logger.warn for consistent logging.
Direct use of console.warn bypasses the logging infrastructure. The logger has an optional warn method that should be used when available.
if (versions.length === 1) {
// print the same depd, but do not stop task
- console.warn(message);
+ if (this.logger.warn) {
+ this.logger.warn(message);
+ } else {
+ this.logger.info('[WARNING] %s', message);
+ }
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.warn(message); | |
| if (versions.length === 1) { | |
| // print the same depd, but do not stop task | |
| if (this.logger.warn) { | |
| this.logger.warn(message); | |
| } else { | |
| this.logger.info('[WARNING] %s', message); | |
| } | |
| } else { |
🤖 Prompt for AI Agents
In lib/proxy_config.ts around line 304, the code currently calls
console.warn(message); replace this direct console call with the logging
infrastructure by invoking the injected logger's warn method when available
(e.g. call logger.warn(message) or use optional chaining/logger presence check),
and if the logger has no warn method fall back to a safe alternative such as
logger.info(message) (or logger.log) so logging remains consistent and no direct
console calls are left.
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the jar2proxy project from JavaScript to TypeScript with comprehensive dependency updates. The migration introduces TypeScript 5.7 with strict type checking, modern tooling (Prettier, ESLint 9), and updates all dependencies to their latest versions. Key infrastructure improvements include replacing the deprecated mkdirp package with Node.js built-in fs.mkdir recursive option and updating the Java build to target Java 8.
- Migrated entire codebase to TypeScript with comprehensive type definitions
- Updated all dependencies to latest stable versions (commander 12.1.0, globby 13.2.2, urllib 4.5.1, etc.)
- Added modern development tooling (TypeScript 5.7, Prettier 3.4, ESLint 9)
Reviewed Changes
Copilot reviewed 16 out of 24 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| package.json | Version bump to 2.0.0 with updated dependencies and new build scripts |
| lib/types.ts | Comprehensive TypeScript type definitions for the entire codebase |
| lib/proxy_config.ts | TypeScript migration of proxy configuration handling |
| lib/nunjucks.ts | TypeScript migration of template engine utilities |
| lib/maven/dependency.ts | TypeScript migration of Maven dependency management |
| lib/logger.ts | TypeScript migration of logging utilities |
| lib/java/platform.ts | TypeScript migration of platform detection |
| lib/java/javahome.ts | TypeScript migration of Java home detection |
| lib/java/jar.ts | TypeScript migration of JAR processing |
| lib/jar2proxy.ts | TypeScript migration of main jar2proxy class |
| eslint.config.js | Modern ESLint 9 flat config with TypeScript support |
| astparser/build.xml | Updated Java build target from 1.6 to 1.8 |
| UPGRADE.md | Comprehensive upgrade guide for v2.0.0 migration |
| .prettierrc | Prettier configuration for code formatting |
| .prettierignore | Prettier ignore patterns |
| .eslintignore | Updated ESLint ignore patterns |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| format: '[jar2proxy-{category}.]YYYY-MM-DD[.log]', | ||
| flushInterval: '1ms', | ||
| timestamp: true, | ||
| seperator: '\n', |
There was a problem hiding this comment.
Corrected spelling of 'seperator' to 'separator'.
| seperator: '\n', | |
| separator: '\n', |
…sions
Major changes:
Breaking changes:
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores