Repository navigation
fix(cli): resolve Node 22 TS entrypoint incompatibility - #1325
Conversation
![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 transitions the CLI entrypoint from a TypeScript file to a bundled JavaScript file (bin/omniroute.mjs). Key changes include updating package.json, adding the bundled file to .gitignore, and implementing a bundling step in scripts/prepublish.ts using esbuild. Feedback focuses on improving the build script's robustness by making bundling failures fatal, ensuring cross-platform compatibility for file permissions, and updating the artifact policy to include the new file.
| try { | ||
| execSync( | ||
| `npx esbuild bin/omniroute.ts --bundle --platform=node --packages=external --format=esm --outfile=bin/omniroute.mjs`, | ||
| { cwd: ROOT, stdio: "inherit" } | ||
| ); | ||
| execSync(`chmod +x bin/omniroute.mjs`, { cwd: ROOT }); | ||
| console.log(" ✅ CLI Entrypoint bundled to bin/omniroute.mjs"); | ||
| } catch (err: any) { | ||
| console.warn(" ⚠️ CLI bundle error:", err.message); | ||
| } |
There was a problem hiding this comment.
The bundling of the CLI entrypoint is a critical build step for the package to function correctly. There are several issues with the current implementation:
- Error Handling: If
esbuildfails, the script currently only logs a warning and continues. Sincepackage.jsonnow points tobin/omniroute.mjs, a failed bundle will result in a broken npm package being published. This should be a fatal error (process.exit(1)). - Cross-platform Compatibility:
execSync('chmod +x ...')will fail on Windows systems, breaking the build for developers on that platform. It should be wrapped in a platform check. - Artifact Policy: The new
bin/omniroute.mjsfile must be added toPACK_ARTIFACT_ROOT_ALLOWED_EXACT_PATHSinscripts/pack-artifact-policy.ts, otherwise thecheck:pack-artifactvalidation (which runs duringprepublishOnly) will fail.
Additionally, it is highly recommended to add esbuild to devDependencies in package.json to ensure build reproducibility and avoid the overhead/unreliability of npx downloading it during the build process.
try {
execSync(
`npx esbuild bin/omniroute.ts --bundle --platform=node --packages=external --format=esm --outfile=bin/omniroute.mjs`,
{ cwd: ROOT, stdio: "inherit" }
);
if (process.platform !== "win32") {
execSync(`chmod +x bin/omniroute.mjs`, { cwd: ROOT });
}
console.log(" ✅ CLI Entrypoint bundled to bin/omniroute.mjs");
} catch (err: any) {
console.error(" ❌ CLI bundle error:", err.message);
process.exit(1);
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d52a6cc7a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| console.log(" 🔨 Bundling CLI Entrypoint (TypeScript → JavaScript)..."); | ||
| try { | ||
| execSync( | ||
| `npx esbuild bin/omniroute.ts --bundle --platform=node --packages=external --format=esm --outfile=bin/omniroute.mjs`, |
There was a problem hiding this comment.
This build step now writes bin/omniroute.mjs, but the pack policy still only allows/requires bin/omniroute.ts in scripts/pack-artifact-policy.ts (PACK_ARTIFACT_ROOT_ALLOWED_EXACT_PATHS and PACK_ARTIFACT_REQUIRED_PATHS). Because prepublishOnly runs build:cli before check:pack-artifact, the generated .mjs is treated as an unexpected artifact and the publish gate fails even when bundling succeeds.
Useful? React with 👍 / 👎.
| execSync(`chmod +x bin/omniroute.mjs`, { cwd: ROOT }); | ||
| console.log(" ✅ CLI Entrypoint bundled to bin/omniroute.mjs"); | ||
| } catch (err: any) { | ||
| console.warn(" ⚠️ CLI bundle error:", err.message); |
There was a problem hiding this comment.
If esbuild (or chmod) fails here, the script only warns and continues, but package.json now points the omniroute bin to bin/omniroute.mjs. In that path, the release can proceed without a valid CLI entrypoint, producing an installed omniroute command that fails at runtime; this should be a hard failure or at least verify the output file exists before continuing.
Useful? React with 👍 / 👎.
CI Coverage Report
Coverage artifact was not available for this run. |
…-entrypoint fix(cli): resolve Node 22 TS entrypoint incompatibility
…-entrypoint fix(cli): resolve Node 22 TS entrypoint incompatibility
1 participant
This PR resolves the
[ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING]error that prevents theomnirouteCLI from correctly running under Node 22. It does this by compiling the CLI TS file into an executablebin/omniroute.mjswrapper during build time.