feat: simplify demo setup and automate renderer builds#1235
feat: simplify demo setup and automate renderer builds#1235wrenj wants to merge 1 commit intogoogle:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request streamlines the setup and execution of Angular-based client samples by introducing automated build hooks, consolidated demo scripts, and clearer documentation. It also addresses peer dependency issues across several renderer packages. Feedback focuses on improving the portability of a suggested sed command in the documentation, optimizing build scripts by removing redundant installation steps, and ensuring consistency by adding a prebuild script alongside the new prestart hook.
|
|
||
| **Tip:** You can quickly set the key from your terminal: | ||
| ```bash | ||
| sed -i '' "s/your_gemini_api_key_here/$GEMINI_API_KEY/" .env |
There was a problem hiding this comment.
The sed command provided in the tip has two issues:
- Portability: The
-i ''syntax is specific to BSDsed(macOS) and will fail on Linux systems (which expect-iwithout the empty string argument, or with a suffix attached). - Delimiter Safety: Using
/as a delimiter forswill cause the command to fail if the$GEMINI_API_KEYcontains a forward slash, which is common in API keys.
A more portable and robust approach is to use a temporary file and a different delimiter like |.
| sed -i '' "s/your_gemini_api_key_here/$GEMINI_API_KEY/" .env | |
| sed "s|your_gemini_api_key_here|$GEMINI_API_KEY|" .env > .env.tmp && mv .env.tmp .env |
| "serve:ssr:rizzcharts": "node dist/rizzcharts/server/server.mjs", | ||
| "serve:ssr:contact": "node dist/contact/server/server.mjs", | ||
| "build:renderer": "cd ../../../renderers && for dir in 'web_core' 'markdown/markdown-it'; do (cd \"$dir\" && npm install && npm run build); done", | ||
| "build:renderer": "cd ../../../renderers && (cd web_core && npm install && npm run build) && (cd markdown/markdown-it && npm install && npm run build) && (cd lit && npm install && npm run build)", |
There was a problem hiding this comment.
Running npm install inside the build:renderer script for web_core and markdown-it is redundant because these packages are already defined as npm workspaces in the root package.json. When the user runs npm install at the root (as instructed in the README), these dependencies are already handled. Removing these extra install steps will significantly speed up the build process.
| "build:renderer": "cd ../../../renderers && (cd web_core && npm install && npm run build) && (cd markdown/markdown-it && npm install && npm run build) && (cd lit && npm install && npm run build)", | |
| "build:renderer": "cd ../../../renderers && (cd web_core && npm run build) && (cd markdown/markdown-it && npm run build) && (cd lit && npm install && npm run build)", |
| "scripts": { | ||
| "ng": "ng", | ||
| "start": "ng serve", | ||
| "prestart": "npm run build:renderer", |
There was a problem hiding this comment.
While adding prestart is helpful for local development, it is recommended to also add a prebuild script. This ensures that the renderers are automatically built before a production build (npm run build), preventing the application from using stale or missing renderer artifacts.
| "prestart": "npm run build:renderer", | |
| "prestart": "npm run build:renderer", | |
| "prebuild": "npm run build:renderer", |
\