chore(packages): fix target dependencies and optimize build process #8608
Conversation
WalkthroughThis pull request enhances the environment configuration and build process for the Gauzy project. Key modifications include updates to the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (6)packages/ui-core/project.json (3)
The build target correctly depends on:
This ensures proper build order and configuration generation.
Using
The development configuration correctly mirrors the production setup, maintaining consistency in the build process. packages/ui-config/project.json (3)
The build target correctly depends on the replace-env target, ensuring environment files are properly configured before the build starts.
The generate-env target properly handles both production and development environments with appropriate commands.
The target correctly depends on generate-env, ensuring proper sequencing of environment setup operations. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
.scripts/configure.ts (1)
Line range hint
383-401: Remove duplicated path resolutionThe path resolution is unnecessarily duplicated in the success callback. The paths are already resolved earlier.
Simplify the callbacks by reusing the already resolved paths:
writeFile(envFilePath, envFileContent, (error) => { if (error) { console.error(`Error writing environment file: ${error}`); } else { - const envFilePath = path.resolve(`./packages/ui-config/src/lib/environments/${envFileDest}`); console.log(`Generated Angular environment file: ${envFilePath}`); } }); writeFile(envFileOtherPath, envFileDestOtherContent, (error) => { if (error) { console.error(`Error writing environment file: ${error}`); } else { - const envFileOtherPath = path.resolve(`./packages/ui-config/src/lib/environments/${envFileDestOther}`); console.log(`Generated Second Empty Angular environment file: ${envFileOtherPath}`); } });
🧹 Nitpick comments (2)
packages/ui-config/project.json (1)
32-46: Consider adding error handling for environment generation.The generate-env target should include error handling to ensure the environment files are generated successfully.
Consider adding error checking:
"generate-env": { "executor": "nx:run-commands", "options": { - "cwd": "." + "cwd": ".", + "parallel": false, + "exitOnError": true }, "configurations": { "production": { - "commands": ["yarn run config:prod"] + "commands": [ + "echo 'Generating production environment...'", + "yarn run config:prod || (echo 'Failed to generate production environment' && exit 1)" + ] },packages/ui-config/scripts/replace-env-files.ts (1)
8-8: Consider using a logging library for better log management.The current console.log statements could be enhanced with a proper logging library for better log levels and formatting.
Consider using winston or debug for more structured logging:
import * as winston from 'winston'; const logger = winston.createLogger({ level: 'info', format: winston.format.combine( winston.format.timestamp(), winston.format.json() ), transports: [ new winston.transports.Console() ] });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.scripts/configure.ts(3 hunks)package.json(1 hunks)packages/ui-config/package.json(1 hunks)packages/ui-config/project.json(2 hunks)packages/ui-config/scripts/replace-env-files.ts(1 hunks)
🔇 Additional comments (5)
packages/ui-config/project.json (1)
12-17: LGTM: Build dependency chain is well structured.
The build target correctly depends on the replace-env target, ensuring environment files are properly set up before building.
packages/ui-config/package.json (1)
22-22: LGTM: Build script properly utilizes nx configuration.
The simplified build command correctly leverages the nx build system and configuration defined in project.json.
.scripts/configure.ts (2)
5-5: LGTM: Necessary imports added for directory handling
The addition of existsSync and mkdirSync imports is appropriate for the new directory handling functionality.
379-382: LGTM: Clean path resolution implementation
The use of path.resolve for constructing file paths is a good practice that ensures consistent path handling across different platforms.
package.json (1)
123-125: LGTM: Build command optimization
The build commands for ui-config have been streamlined by removing redundant config:dev calls. This is a good optimization that should improve build times.
Let's verify that the removed config:dev step isn't required elsewhere:
✅ Verification successful
LGTM: Build command optimization is safe
The removal of config:dev from ui-config build commands is safe. The search results show that:
- ui-config package builds do not depend on config:dev
- Other packages that depend on ui-config (like ui-core) reference the build commands directly
- The only config:dev dependency is in the start:gauzy:forever command which is unrelated to the build process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any other scripts depend on the config:dev step for ui-config
rg "yarn run config:dev.*build:package:ui-config" --type yaml --type json
Length of output: 75
Script:
#!/bin/bash
# Let's check for any references to config:dev in relation to ui-config more broadly
rg "config:dev.*ui-config|ui-config.*config:dev" --type yaml --type json -A 2
# Also check for any direct dependencies on config:dev in package.json
rg "config:dev" package.json -A 2
# Check if there are any workflow files that might reference these build steps
fd -e yml -e yaml . .github/workflows -x cat {}
Length of output: 69247
Script:
#!/bin/bash
# Let's check for any references to config:dev in build commands or dependencies
rg "config:dev.*build|build.*config:dev" package.json -A 2
# Also check for any direct dependencies between ui-config and config:dev
rg "ui-config.*config:dev|config:dev.*ui-config" package.json -A 2
# Check for any references to ui-config in the build scripts
rg "build:package:ui-config" package.json -A 2
Length of output: 4260
|
|
||
| const env = process.env['NODE_ENV'] || 'development'; |
There was a problem hiding this comment.
Add error handling and environment argument parsing.
The script needs improvements in error handling and should handle the --environment argument passed from project.json.
Consider these improvements:
+ import { argv } from 'process';
+
+ // Parse environment argument
+ const args = argv.slice(2);
+ const envArg = args.find(arg => arg.startsWith('--environment='))?.split('=')[1];
+
- const env = process.env['NODE_ENV'] || 'development';
+ const env = envArg || process.env['NODE_ENV'] || 'development';
console.log(`Current NODE_ENV: ${process.env['NODE_ENV']}`);
if (env === 'production') {
console.log('Replacing environment files for __dirname: ' + __dirname);
- const sourceFile = path.join(path.resolve(__dirname, '../src/lib/environments'), 'environment.prod.ts');
- const targetFile = path.join(path.resolve(__dirname, '../src/lib/environments'), 'environment.ts');
+ const envDir = path.resolve(__dirname, '../src/lib/environments');
+
+ // Verify directory exists
+ if (!fs.existsSync(envDir)) {
+ console.error(`Environment directory not found: ${envDir}`);
+ process.exit(1);
+ }
+
+ const sourceFile = path.join(envDir, 'environment.prod.ts');
+ const targetFile = path.join(envDir, 'environment.ts');
+
+ // Verify source file exists
+ if (!fs.existsSync(sourceFile)) {
+ console.error(`Source file not found: ${sourceFile}`);
+ process.exit(1);
+ }Also applies to: 14-16
| // Define the folder paths | ||
| const environmentsFolder = './packages/ui-config/src/lib/environments'; | ||
|
|
||
| // Ensure the environments folder exists | ||
| if (!existsSync(environmentsFolder)) { | ||
| mkdirSync(environmentsFolder, { recursive: true }); | ||
| console.log(`Created environments folder: ${environmentsFolder}`); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory creation
While the directory creation logic is good, it should include error handling for scenarios where directory creation fails.
Consider wrapping the directory creation in a try-catch block:
- if (!existsSync(environmentsFolder)) {
- mkdirSync(environmentsFolder, { recursive: true });
- console.log(`Created environments folder: ${environmentsFolder}`);
- }
+ if (!existsSync(environmentsFolder)) {
+ try {
+ mkdirSync(environmentsFolder, { recursive: true });
+ console.log(`Created environments folder: ${environmentsFolder}`);
+ } catch (error) {
+ console.error(`Error creating environments folder: ${error}`);
+ process.exit(1);
+ }
+ }📝 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.
| // Define the folder paths | |
| const environmentsFolder = './packages/ui-config/src/lib/environments'; | |
| // Ensure the environments folder exists | |
| if (!existsSync(environmentsFolder)) { | |
| mkdirSync(environmentsFolder, { recursive: true }); | |
| console.log(`Created environments folder: ${environmentsFolder}`); | |
| } | |
| // Define the folder paths | |
| const environmentsFolder = './packages/ui-config/src/lib/environments'; | |
| // Ensure the environments folder exists | |
| if (!existsSync(environmentsFolder)) { | |
| try { | |
| mkdirSync(environmentsFolder, { recursive: true }); | |
| console.log(`Created environments folder: ${environmentsFolder}`); | |
| } catch (error) { | |
| console.error(`Error creating environments folder: ${error}`); | |
| process.exit(1); | |
| } | |
| } |
|
View your CI Pipeline Execution ↗ for commit adc6e96.
☁️ Nx Cloud last updated this comment at |
rahul-rocket
changed the title
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by CodeRabbit
New Features
Bug Fixes
Chores