Document ES module postinstall fix and add file extension guidelines#71
Document ES module postinstall fix and add file extension guidelines#71
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Comment |
… configured Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
Co-authored-by: clduab11 <185000089+clduab11@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR documents the ES module/CommonJS handling in the project to prevent regressions. The postinstall script was already using the correct .cjs extension to avoid ES module scope errors, but this wasn't documented, creating risk of future mistakes.
Key changes:
- Added file extension guidelines explaining when to use
.mjs,.cjs, and why to avoid.js - Documented that
postinstall.cjsprevents the "require is not defined" error in ES module scope
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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".
|
@gemini-code-assist, review and analyze all changes including resolved comments in this PR. |
There was a problem hiding this comment.
Code Review
This pull request introduces a new extension management system for the Gemini CLI, which is a significant and valuable addition. It also includes documentation updates regarding ES module conventions, which helps prevent future regressions. My review focuses on the new extension manager feature. I've identified several critical security vulnerabilities, including potential for command injection and path traversal, which must be addressed. I've also noted some other areas for improvement regarding usability and error handling. Please see my detailed comments for suggestions on how to resolve these issues.
Users reported
npm install -gfailing withReferenceError: require is not defined in ES module scopewhen installing the package. The fix was already implemented (postinstall script uses.cjsextension), but lacked documentation to prevent regression.Changes
Documentation
scripts/README.md:.mjsfor ES modules (import/export).cjsfor CommonJS (require/module.exports).jswith"type": "module"to prevent ambiguityContext
Package.json has
"type": "module", making all.jsfiles ES modules by default. The postinstall script requires CommonJS forrequire()statements, hence the.cjsextension.All scripts now follow the convention: 5
.mjsfiles, 1.cjsfile, zero ambiguous.jsfiles.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.