Skip to content
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

chore: Move the dependencies into individual packages #631

Closed

Conversation

bansal-harsh-2504
Copy link
Contributor

@bansal-harsh-2504 bansal-harsh-2504 commented Jan 14, 2025

User description

Description

moved all the dependencies from root package.json to their respective packages

Fixes #530

Mentions

@rajdip-b

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

Documentation Update

  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement, configuration changes


Description

  • Moved dependencies from root package.json to individual packages.

  • Updated tsconfig.json paths for schema and api-client packages.

  • Adjusted build scripts and dependencies in cli and api-client.

  • Added new dependencies and devDependencies to relevant package.json files.


Changes walkthrough 📝

Relevant files
Configuration changes
7 files
package.json
Reorganized dependencies specific to the API package         
+11/-2   
package.json
Updated build script and added dependencies                           
+3/-1     
package.json
Added dependencies and devDependencies for API client       
+9/-0     
package.json
Added dependencies and devDependencies for schema package
+7/-1     
package.json
Added schema dependency to platform package                           
+1/-0     
tsconfig.json
Updated tsconfig path for schema package                                 
+1/-1     
tsconfig.json
Updated tsconfig path for API client package                         
+1/-1     
Additional files
2 files
package.json +0/-48   
pnpm-lock.yaml +1460/-3322

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

Copy link
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

530 - Fully compliant

Compliant requirements:

  • Move package-specific dependencies from root package.json into individual package.json files
  • Keep shared dependencies in individual packages even if used by multiple packages
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Dependency Placement

Jest and its types are added as both runtime dependencies and devDependencies. Testing libraries should only be in devDependencies.

  "jest": "^29.7.0",
  "zod": "^3.23.6",
  "tsc-alias": "^1.8.10"
},
"devDependencies": {
  "@types/jest": "^29.5.2",
  "eslint-plugin-prettier": "^5.0.0"
Dependency Placement

Jest and @types/node are added as runtime dependencies but should be in devDependencies since they are only needed during development.

  "@keyshade/schema": "workspace:*",
  "@types/node": "^20.14.10",
  "jest": "^29.7.0",
  "tsc-alias": "^1.8.10"
},
"devDependencies": {
  "@types/jest": "^29.5.2"
}

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Score
Possible issue
Maintain consistent build order across platforms by building ESM before CJS

The build script order matters for cross-platform compatibility. The current change
swaps the order of ESM and CJS builds which could cause issues. Keep the original
order with ESM first.

apps/cli/package.json [17]

-"build": "esbuild src/index.ts --bundle --platform=node --outfile=dist/index.cjs --format=cjs --banner:js=\"#!/usr/bin/env node\" && esbuild src/index.ts --bundle --platform=node --outfile=dist/index.esm.js --format=esm --banner:js=\"#!/usr/bin/env node\"",
+"build": "esbuild src/index.ts --bundle --platform=node --outfile=dist/index.esm.js --format=esm --banner:js=\"#!/usr/bin/env node\" && esbuild src/index.ts --bundle --platform=node --outfile=dist/index.cjs --format=cjs --banner:js=\"#!/usr/bin/env node\"",
  • Apply this suggestion
Suggestion importance[1-10]: 7

Why: The order of build operations can impact cross-platform compatibility. Building ESM first followed by CJS is a more reliable approach that matches the original implementation.

7

@bansal-harsh-2504 bansal-harsh-2504 changed the title Move the dependencies into individual packages #530 chore: Move the dependencies into individual packages Jan 14, 2025
@rajdip-b
Copy link
Member

@bansal-harsh-2504 can you update the lockfile?

@bansal-harsh-2504
Copy link
Contributor Author

bansal-harsh-2504 commented Jan 14, 2025

when i do pnpm i
it outputs

Scope: all 10 workspace projects
Lockfile is up to date, resolution step is skipped
Already up to date

@rajdip-b

@rajdip-b
Copy link
Member

There are still a few errors in the packages. try running the pnpm commands and you will notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move the dependencies into individual packages
2 participants