-
Notifications
You must be signed in to change notification settings - Fork 9.2k
bundling #11633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
bundling #11633
Conversation
fix: Update NOTICES.txt to reflect current dependencies
…tive mode (#10194) Co-authored-by: Allen Hutchison <adh@google.com>
This commit unifies the bundling and publishing process for the CLI package across both npmjs.org and GitHub Packages. - The script has been updated to only rename packages for the GitHub registry, removing the bundle copying and package.json modification logic. - The has been modified to remove the flag from the CLI publish step, ensuring the root package (which includes the bundled executable) is published. This ensures a consistent, bundled artifact is published to both registries.
This PR introduces a new, automated workflow for developers to switch between the development (GitHub Packages) and production (npmjs.org) npm registries. - A new script, , now automatically backs up and modifies the user's global file. - has been updated with and scripts to run this new script. - has been updated to reflect this new, simplified, and automated process, removing all manual steps for the user.
This commit adds a step to the E2E workflow to create an .npmrc file and configure it with the necessary credentials for the GitHub Packages registry. This resolves the '401 Unauthorized' error that was occurring during the 'npm ci' step.
Summary of ChangesHello @mattKorwel, 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 significantly refines the project's development and release processes, primarily by overhauling npm package management and CLI bundling. It introduces automated scripts for developers to seamlessly configure their local npm registries, enabling easy access to pre-release versions hosted on GitHub Packages. Concurrently, the changes simplify the release preparation workflow by adjusting how the CLI package is handled and published, moving towards a model where a pre-bundled CLI is consumed as a dependency. Additionally, the update includes minor but important improvements to the CLI's command loading capabilities and accessibility features, alongside a default setting adjustment for an experimental feature. Highlights
Ignored Files
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces significant changes to the project's bundling and publishing strategy, shifting to GitHub Packages for pre-release versions under the new @google-gemini scope. New scripts have been added to manage npm registry configuration for developers. My review found a critical issue in the new configure-registry.js script that could lead to the loss of a user's global .npmrc configuration. I've provided a detailed comment and a suggested fix to make the process safer by using a backup-and-restore mechanism. The other changes, including updates to CI/CD, documentation, and tests, appear consistent with the overall goal of this PR.
TLDR
Bundling
The primary benefits of bundling for a command-line tool revolve around performance, reliability, and a better end-user experience (Expand for more info)
Why It's a Good Change for a CLI (Pros)
1. 🚀 Faster Startup Performance:
This is the most significant advantage for a CLI. When you run a Node.js application, the
require()orimportstatements trigger a complex module resolution process that involves searching the file system fornode_modulesdirectories. This can result in hundreds or even thousands of file system I/O operations, which slows down the initial startup time.gemini -> require('libA') -> require('libB') -> ...(many file lookups)gemini(one file is loaded into memory)By consolidating all the necessary code into a single file, you reduce the startup to a single file read, making the CLI feel significantly more responsive to the user.
2. 📦 Faster and More Reliable Installation:
Users installing your CLI will have a much better experience.
npm installhas to download and write far fewer files to the user's disk. This is faster and reduces the chance of installation errors due to network hiccups or file system issues.node_modulesdirectory.3. 🛡️ Increased Reliability and Consistency:
A bundled application is self-contained and predictable.
node_modulesIssues: Your CLI will no longer be affected by the user's localnode_modulesstructure, global packages, or potential conflicts between different projects. It just runs.4. 🚚 Simplified Distribution:
Having a single executable file makes distribution and packaging much simpler if you ever decide to, for example, package the CLI with a standalone Node.js binary (using a tool like
pkg) or include it in a Docker image.The Downsides of Bundling (Cons)
While the benefits are substantial for a CLI, there are trade-offs to consider, mostly affecting the development and debugging workflow.
1. 🐛 Debugging Complexity:
The code that runs is not the code you wrote. It's been transpiled, minified, and combined.
.js.mapfiles). A bundler can generate source maps that map the bundled code back to your original source files. When you use these with developer tools, stack traces become readable again. You can choose to publish these for public debugging or keep them private for your team.2. 🩹 Difficult for End-Users to Patch:
If a user discovers a bug in one of your dependencies, they can't easily use a tool like
patch-packageto fix it themselves, because the dependency code is baked into the bundle. For a CLI, this is generally an acceptable trade-off for the reliability it provides.3. 🏗️ Added Build-Step Complexity:
Your development process now requires a build step. You can't just run the source code directly; you must first build the bundle. This adds a layer of complexity to your local development, testing, and CI/CD pipeline.
Summary: Pros vs. Cons
Conclusion:
For a library that other developers will include in their projects, bundling is often a bad idea because it leads to code duplication and conflicts.
However, for a standalone application like the Gemini CLI, the benefits of performance and reliability for the end-user are paramount. The faster startup time and insulation from environmental issues create a much more professional and robust tool. The downsides are primarily developer-facing and can be effectively managed with source maps and a solid build process. Your PR is making a very good trade-off.
CI
Artifacts
Both of those artifacts are then used in all remaining tests. Specifically end 2 end tests no longer build the code or the sandbox image (with in context of github actions, this should all still work locally).
Merge queue Skipper
While i was hopeful that this would have a net positive impact on us, it hasn't materialized. It is currently adding a significant overhead in code complexity and maintenance. This PR removes it in the spirit of simplification. If we want to approach it again later we can, totally open to feedback here.
Environments
All the ci operations work against our
prodenvironment. No changes will be made or packages pushed toprod. This is intended to help reduce noise, customer facing confusion, and churn on pakcage versions (which are onetime use on npmjs). We will follow this change with the ability to 'promote' a specific tested bundled package fromdevtoprodand ideally this would be our primmary deployment mechanism. All patches, releases etc will go do dev, we will validate and we can delete and reuse package numbers if needed. Then when we are comfortable those will be promoted to Prod. This will be a follow on change.Release Channels
As of version 0.11 stable, and 0.12-preview the code is still non bundled. 0.13-nightly is bundled.