Feature/Production dependency Optimization By running the command 'npm prune --omit=dev' #2848
Conversation
WalkthroughThe pull request introduces a comprehensive dependency management solution for Node.js projects. It includes three new scripts and a README document in the Changes
Assessment against linked issues
Possibly related PRs
Poem
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
package.json (1)
33-33: Add a dry-run option for safety.The script makes significant changes to dependencies. A dry-run option would help users preview changes before applying them.
-"move-and-prune": "./deps/analyze-dependencies.sh && ./deps/move-from-diff.sh && npm prune --omit=dev" +"move-and-prune": "./deps/analyze-dependencies.sh && ./deps/move-from-diff.sh && npm prune --omit=dev", +"move-and-prune:dry-run": "DRY_RUN=true ./deps/analyze-dependencies.sh && DRY_RUN=true ./deps/move-from-diff.sh"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deps/analyze-dependencies.sh(1 hunks)deps/move-from-diff.sh(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check for linting, formatting, and type errors
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
package.json (1)
33-33: 🛠️ Refactor suggestionEnsure script permissions in CI/CD environments.
The new script might fail in CI/CD environments if the shell scripts don't have execute permissions.
Consider adding a preinstall script to set permissions:
+"preinstall": "chmod +x ./deps/analyze-dependencies.sh ./deps/move-from-diff.sh",
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2848 +/- ##
===========================================
- Coverage 97.72% 97.68% -0.05%
===========================================
Files 364 364
Lines 18569 18563 -6
Branches 2682 2677 -5
===========================================
- Hits 18147 18133 -14
- Misses 417 425 +8
Partials 5 5 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
deps/move-from-diff.sh (3)
74-87: Add error handling for grep command.While the simple grep pattern is preferred (as per learnings), the command should handle potential errors.
- if grep -qr "$PACKAGE" ./src; then + if grep -qr "$PACKAGE" ./src 2>/dev/null; then echo "$PACKAGE is required in production." echo "$PACKAGE" >> "$REQUIRED_FOR_PRODUCTION_FILE" else - echo "$PACKAGE is not used in production." + if [ $? -eq 2 ]; then + echo "Error occurred while searching for $PACKAGE" + exit 1 + else + echo "$PACKAGE is not used in production." + fi fi
98-115: Consider making dependency moves atomic.If a failure occurs mid-process, some dependencies might be left in an inconsistent state.
Consider creating a backup of package.json before starting and restore it on failure:
+# Create backup of package.json +cp package.json package.json.bak + while IFS= read -r PACKAGE; do echo "Moving $PACKAGE to dependencies..." if ! npm uninstall "$PACKAGE" --save-dev; then echo "Error: Failed to uninstall $PACKAGE from devDependencies" + mv package.json.bak package.json exit 1 fi if ! npm install "$PACKAGE" --save-prod; then echo "Error: Failed to install $PACKAGE as production dependency" echo "Warning: Package is now removed from both devDependencies and dependencies" + mv package.json.bak package.json exit 1 fi done < "$REQUIRED_FOR_PRODUCTION_FILE" + +# Remove backup if successful +rm package.json.bak
116-119: Enhance success message with details.Consider providing more information about what was moved.
-echo "Required dev dependencies successfully moved to production dependencies." +echo "Successfully moved the following dev dependencies to production dependencies:" +cat "$REQUIRED_FOR_PRODUCTION_FILE" rm -f "$REQUIRED_FOR_PRODUCTION_FILE" +echo "Operation completed successfully."deps/analyze-dependencies.sh (2)
110-124: Add package.json validation before analysis.Consider validating package.json format before proceeding with dependency analysis.
+echo "Validating package.json format..." +if ! jq '.' package.json >/dev/null 2>&1; then + echo "Error: Invalid package.json format" + exit 1 +fi + echo "Generating list of production dependencies (top-level only)..."
126-137: Consider pretty-printing JSON output.The JSON files could be more readable for manual inspection.
-jq '.dependencies? // {} | keys' "$DEPS_FOLDER/prod-deps.json" > "$DEPS_FOLDER/prod-deps-keys.json" +jq -r '.dependencies? // {} | keys | .[] | select(length > 0)' "$DEPS_FOLDER/prod-deps.json" | sort > "$DEPS_FOLDER/prod-deps-keys.json"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deps/analyze-dependencies.sh(1 hunks)deps/move-from-diff.sh(1 hunks)
🧰 Additional context used
📓 Learnings (2)
deps/move-from-diff.sh (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2848
File: deps/move-from-diff.sh:81-81
Timestamp: 2025-01-11T21:35:22.240Z
Learning: In the talawa-api project, the simple grep pattern `grep -qr "$PACKAGE" ./src ./lib ./app` is more reliable for detecting package usage compared to specific import/require patterns.
deps/analyze-dependencies.sh (2)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2848
File: deps/analyze-dependencies.sh:56-62
Timestamp: 2025-01-11T20:56:31.406Z
Learning: For the `analyze-dependencies.sh` script in talawa-api, `jq` installation should be documented as a prerequisite in the README rather than attempting automatic installation in the script.
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2848
File: deps/analyze-dependencies.sh:87-87
Timestamp: 2025-01-11T20:48:38.012Z
Learning: In shell scripts dealing with Node.js projects, always check for .env file existence, load environment variables from it, and verify NODE_ENV is set before making environment-specific decisions. Default to "development" if NODE_ENV is not set.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (4)
deps/move-from-diff.sh (2)
49-69: LGTM! Robust error handling and file checks.The script properly handles errors and validates prerequisites before proceeding.
45-46:⚠️ Potential issueDocumentation and implementation mismatch for source directories.
The documentation mentions searching in
./src,./lib, and./appfolders, but the implementation only searches in./src. This could lead to missed package references.Update the implementation to match the documentation or vice versa:
- if grep -qr "$PACKAGE" ./src; then + if grep -qr "$PACKAGE" ./src ./lib ./app 2>/dev/null; thenLikely invalid or redundant comment.
deps/analyze-dependencies.sh (2)
1-52: LGTM! Well-documented with clear prerequisites.The documentation is comprehensive and follows the learning about documenting jq installation as a prerequisite.
87-108: LGTM! Robust environment handling.The script follows best practices by:
- Checking for .env file
- Setting default NODE_ENV
- Avoiding npm install in production
|
@meetulr @varshith257 PTAL |
|
Looks good. Can you move readme of both scripts to new |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
scripts/deps/analyze-dependencies.sh (2)
4-22: Add version check for jq compatibility.While the script checks for jq's presence, it doesn't verify the version. Consider adding a version check to ensure compatibility.
if ! command -v jq >/dev/null 2>&1; then echo "jq is not installed." +else + JQ_VERSION=$(jq --version) + echo "Found $JQ_VERSION" + # Verify minimum version (e.g., 1.6) + if ! echo "$JQ_VERSION" | grep -q "jq-1\.[6-9]"; then + echo "Warning: This script is tested with jq 1.6+. Your version may not be compatible." + fi fi
58-73: Add cleanup step for old JSON files.Consider cleaning up old JSON files before generating new ones to prevent stale data from affecting the analysis.
+echo "Cleaning up old dependency files..." +rm -f "$DEPS_FOLDER/prod-deps.json" "$DEPS_FOLDER/dev-deps.json" \ + "$DEPS_FOLDER/prod-deps-keys.json" "$DEPS_FOLDER/dev-deps-keys.json" + echo "Generating list of production dependencies (top-level only)..."scripts/deps/README.md (1)
1-44: Fix grammatical issues in the documentation.A few grammatical improvements are needed:
- Line 25: "MacOS" should be "macOS"
- Line 29: Add "to" before "Make" ("to make the scripts executable")
🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: The operating system from Apple is written “macOS”.
Context: .../CentOS**:yum install -y jq- MacOS (with Homebrew):brew install jq...(MAC_OS)
[grammar] ~29-~29: It seems that “to” is missing before the verb.
Context: ... Ensure Scripts Are Executable - Make the scripts executable: ```sh ...(MISSING_TO_BETWEEN_BE_AND_VB)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
package.json(1 hunks)scripts/deps/README.md(1 hunks)scripts/deps/analyze-dependencies.sh(1 hunks)scripts/deps/move-from-diff.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 LanguageTool
scripts/deps/README.md
[grammar] ~25-~25: The operating system from Apple is written “macOS”.
Context: .../CentOS**: yum install -y jq - MacOS (with Homebrew): brew install jq ...
(MAC_OS)
[grammar] ~29-~29: It seems that “to” is missing before the verb.
Context: ... Ensure Scripts Are Executable - Make the scripts executable: ```sh ...
(MISSING_TO_BETWEEN_BE_AND_VB)
[uncategorized] ~52-~52: Possible missing preposition found.
Context: ...analyze-dependencies.sh Script** - Navigate to the project root directory (where `p...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...n dependencies. - dev-deps.json: Top-level dev+prod dependencies. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ndencies. - prod-deps-keys.json: Keys of production dependencies. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...endencies. - dev-deps-keys.json: Keys of dev+prod dependencies. - D...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (5)
scripts/deps/analyze-dependencies.sh (1)
74-85: LGTM! Clear output format and proper error handling.The comparison logic is well-implemented with proper sorting and diff visualization.
scripts/deps/move-from-diff.sh (2)
1-23: LGTM! Proper initialization and error handling.The script correctly checks for prerequisites and handles errors appropriately.
69-72: LGTM! Proper cleanup of temporary files.The script properly cleans up temporary files and provides clear success messaging.
scripts/deps/README.md (2)
47-97: LGTM! Clear and comprehensive usage instructions.The documentation provides clear step-by-step instructions with detailed explanations of each script's purpose and output.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~52-~52: Possible missing preposition found.
Context: ...analyze-dependencies.shScript** - Navigate to the project root directory (where `p...(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...n dependencies. -dev-deps.json: Top-level dev+prod dependencies. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ndencies. -prod-deps-keys.json: Keys of production dependencies. ...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...endencies. -dev-deps-keys.json: Keys of dev+prod dependencies. - D...(UNLIKELY_OPENING_PUNCTUATION)
98-114: LGTM! Important notes about testing and deployment.The documentation properly emphasizes the importance of local testing and provides clear instructions for automated execution.
|
Also, is this necessary? This issue addresses an uncommon scenario where a production dependency is misclassified as a development dependency. A more common scenario would be a development dependency being added as a production dependency @varshith257 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
scripts/deps/analyze-dependencies.sh (3)
31-33: Add error handling for directory creation.The mkdir command should include error handling in case of permission issues or other failures.
DEPS_FOLDER="./scripts/deps" echo "Creating the $DEPS_FOLDER folder for storing dependency files..." -mkdir -p "$DEPS_FOLDER" +if ! mkdir -p "$DEPS_FOLDER"; then + echo "Error: Failed to create $DEPS_FOLDER directory" + exit 1 +fi
35-40: Add validation for loaded environment variables.After loading the .env file, validate that required environment variables are properly set and contain valid values.
if [ -f ".env" ]; then . ./.env + # Validate required environment variables + for var in NODE_ENV DATABASE_URL API_KEY; do + if [ -n "${!var}" ]; then + echo "Loaded $var from .env" + fi + done else echo "Warning: .env file not found" fi
68-78: Add validation for jq output.Verify that the jq command produced valid JSON output before proceeding with the comparison.
echo "Extracting top-level dependencies from prod-deps.json..." -jq '.dependencies? // {} | keys' "$DEPS_FOLDER/prod-deps.json" > "$DEPS_FOLDER/prod-deps-keys.json" +if ! jq '.dependencies? // {} | keys' "$DEPS_FOLDER/prod-deps.json" > "$DEPS_FOLDER/prod-deps-keys.json"; then + echo "Error: Failed to process prod-deps.json" + exit 1 +fi + +# Validate JSON output +if ! jq empty "$DEPS_FOLDER/prod-deps-keys.json" 2>/dev/null; then + echo "Error: Invalid JSON output in prod-deps-keys.json" + exit 1 +fiscripts/deps/README.md (2)
25-25: Fix macOS capitalization.The operating system from Apple should be written as "macOS" instead of "MacOS".
- - **MacOS (with Homebrew)**: `brew install jq` + - **macOS (with Homebrew)**: `brew install jq`🧰 Tools
🪛 LanguageTool
[grammar] ~25-~25: The operating system from Apple is written “macOS”.
Context: .../CentOS**:yum install -y jq- MacOS (with Homebrew):brew install jq...(MAC_OS)
113-117: Add a troubleshooting section to the documentation.Consider adding a troubleshooting section to help users resolve common issues they might encounter while using these scripts.
Add the following section:
## Troubleshooting ### Common Issues 1. **Permission Denied** - Ensure scripts are executable using `chmod +x` - Verify you have write permissions in the target directories 2. **jq Command Not Found** - Verify jq installation using `which jq` - Follow installation instructions in Prerequisites section 3. **npm ls Errors** - Some errors are expected and won't affect the analysis - If you see "ELSPROBLEMS", verify your package.json is valid 4. **Invalid JSON Output** - Check if your package.json is properly formatted - Ensure no manual modifications were made to generated files
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
scripts/deps/README.md(1 hunks)scripts/deps/analyze-dependencies.sh(1 hunks)scripts/deps/move-from-diff.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/deps/move-from-diff.sh
🧰 Additional context used
📓 Learnings (1)
scripts/deps/analyze-dependencies.sh (1)
Learnt from: PurnenduMIshra129th
PR: PalisadoesFoundation/talawa-api#2848
File: deps/analyze-dependencies.sh:56-62
Timestamp: 2025-01-11T20:56:31.406Z
Learning: For the `analyze-dependencies.sh` script in talawa-api, `jq` installation should be documented as a prerequisite in the README rather than attempting automatic installation in the script.
🪛 LanguageTool
scripts/deps/README.md
[grammar] ~25-~25: The operating system from Apple is written “macOS”.
Context: .../CentOS**: yum install -y jq - MacOS (with Homebrew): brew install jq ...
(MAC_OS)
[grammar] ~29-~29: It seems that “to” is missing before the verb.
Context: ... Ensure Scripts Are Executable - Make the scripts executable: ```sh ...
(MISSING_TO_BETWEEN_BE_AND_VB)
[uncategorized] ~52-~52: Possible missing preposition found.
Context: ...analyze-dependencies.sh Script** - Navigate to the project root directory (where `p...
(AI_HYDRA_LEO_MISSING_TO)
[uncategorized] ~60-~60: Loose punctuation mark.
Context: ...n dependencies. - dev-deps.json: Top-level dev+prod dependencies. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~61-~61: Loose punctuation mark.
Context: ...ndencies. - prod-deps-keys.json: Keys of production dependencies. ...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~62-~62: Loose punctuation mark.
Context: ...endencies. - dev-deps-keys.json: Keys of dev+prod dependencies. - D...
(UNLIKELY_OPENING_PUNCTUATION)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Testing Application (22.x)
- GitHub Check: Analyse Code With CodeQL (typescript)
🔇 Additional comments (1)
scripts/deps/analyze-dependencies.sh (1)
80-85: LGTM! Clear and helpful output format.The summary section provides clear information about the generated files and helpful instructions for manual comparison.
|
@varshith257 @palisadoes check once suggestion are now implemented |
|
Is it only 3 dependencies are differentiation b/w dev and prod envs? |
@meetulr Yes, they are not needed. After going through all opened issues and discussion of related work the issue raised from dev env consuming too much resources(imo this is what dev env made for) and when dealing with prod it's even better if we go with what build made from dev than this experimental optimisations in searching of solutions of uncommon solutions |
|
The last production image we worked with was ~190MB which is already quite optimized. I believe it would be more beneficial to focus on fixing the current build issue rather than experimenting with optimizations. The production env is like an acidic component derived from the dev env, if we experiment with it too much we risk destabilizing the prod output. I think stability should remain our priority |
|
@palisadoes Can you detail in few lines what's you are looking for? Is it concern with image size used in VPS Cloud ? OR Is it concern with the developers setting in locally |
|
The develop build was too resource hungry for many contributors to use on their laptops so I challenged you so to improve the performance. The discussion discovered that the production build was broken. Rather than fixing it the conversation went to improving the dev build performance as it didn't make sense spending too much time on working on the develop branch as it would soon be replaced by the postgres approach. Then we fixed the prod build in develop. And we decide that maybe it would be best to go with that instead because it confirmed fewer resources and was therefore more usable. Then it was proposed that we could spend the time to get dev more efficient anyway. So we are now at the point where we have two optimized builds, one of which could be unstable based on my interpretation of the threads. Dev was so poor in performance that we couldn't run it on our cloud server. We need a build for the cloud server that is efficient. Now that prod works, I think we have that. This is what should be deployed there. We need a dev environment that all developers can use without crippling their machine performance. It doesn't need to be perfect, just usable and stable. Developers will need the iGraphQL that I think it provides. What we don't want is contributors trying the app and getting discouraged and leaving because the build's resource intensity causes their systems to hang. The develop branch will have break / fix issues that will need to be addressed as people evaluate Talawa on the cloud server. Postgres won't be stable for a few months. Therefore we need both builds in develop with the characteristics mentioned previously. |
|
Yes, dev build has been integrated into Cloud temporarily till prod build is fixed. Now I think @vasujain275 have also switched to prod image. I didn't get it what happened with dev build. Mostly the hungry of resources with dev build depends on packages it installed for what I know if it's manual setup and docker as we already good with multi-stage builds for what I far known is around 1.9 GB when docker dev is started. Idk after integration of minio etc.. Let me have opinions of new contributors in slack of what they facing |
|
@varshith257 Yes I have switched to prod image (~500mb) for the cloud instance. It is working perfectly om vps, just caddy tls issue is left to figure out |
|
Then can we close this optimisation issues opened? cc: @meetulr @palisadoes |
|
@vasujain275 Isn't it approx 200 MB? This is the last when I integrated prod image size |
|
@varshith257 I have recently made many changes to the production docker file to make it work effectively. I don't remember the current size right now, will get back to you once the deployment is completed. |
|
@palisadoes @meetulr @varshith257 @vasujain275 in my case when i build the docker image in production it is around 500mb only our talawa-api code but after removing the development dependency it is around 300 mb.So after this you can decide whether or not this optimization tecnique is useful or not . And about resource hungry topic my observation our maximum resource are consumed by mongodb. Not our talawa-api code base . If you consider one thing is thing is in our development branch we really need to work on database optimization. And if possible we can turn off database logging as it is logging in every second due to this also it consumes most resources. I observe in one case when the services start the mongodb at that time it overwhelmed the system. |
|
I'm going to close this.
|
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #2834
If relevant, did you update the documentation?
Yes i added comments on scripts. But did not update the documentation.
Summary
-Previously npm prune --omit=dev commands deletes some necessary production dependency when we run because these dependency are listed under development.
-So i have created script which will dynamically cheack which dependency are required for our production and remove all the development dependency .
-As a result all unused packages will remove from our production which results smaller node_modules size and better performance.
Does this PR introduce a breaking change?
No
Checklist
CodeRabbit AI Review
Test Coverage
Other information
-Please ensure when user run the script by package.json
npm run move-and-prunethe script will run . So before make sure user have execute permission for the script.-I have added command for both the script.Please ensure you go through these readme section once before you run the command.
Have you read the contributing guide?
Yes
Summary by CodeRabbit
New Features
move-and-pruneDocumentation
Chores