search

keyshade-xyz / keyshade

Realtime secret and configuration management tool, with the best in class security and seamless integration support
https://keyshade.xyz
Mozilla Public License 2.0
196 stars 96 forks source link

Migrate CLI to Bun #400

Open RanitMukherjee opened 2 months ago

RanitMukherjee commented 2 months ago

User description

Description

As of now, only have modified the package.json of the CLI

Fixes #363

Future Improvements

This is surely incomplete as of now & the only motivation behind such a low effort PR is to understand the requirements better & actually get the work done.

Mentions

@rajdip-b

Developer's checklist

If changes are made in the code:

Documentation Update

PR Type

enhancement, configuration changes

Description

Changes walkthrough 📝

Relevant files
Configuration changes
package.json
Add Bun support to CLI scripts in package.json                     
apps/cli/package.json
  • Added new scripts for Bun build, start, and dev commands.
  • Modified existing dev script to include Bun commands.
    		•  +5/-2     

    💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ No key issues to review
    codiumai-pr-agent-free[bot] commented 2 months ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Correct the command typo in the "bun:dev" script ___ **The script "bun:dev" is currently defined as "bun bun:build && bun
    dist/src/index.js". This is likely a typo or error, as "bun bun:build" is not a
    valid command. It should be corrected to "bun build" to match the "bun:build" script
    definition.** [apps/cli/package.json [13]](https://github.com/keyshade-xyz/keyshade/pull/400/files#diff-1625a7848c01883721bcd146381e2a6bff3c03814d4586559265d8e54a0c9565R13-R13) ```diff -"bun:dev": "bun bun:build && bun dist/src/index.js" +"bun:dev": "bun build && bun dist/src/index.js" ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 10 Why: The suggestion corrects a clear typo in the "bun:dev" script, ensuring the command is valid and executable.
    		10
    Enhancement
    Update the "dev" script to use "bun" for building, ensuring consistency ___ **The "dev" script uses "pnpm" for building which might not be consistent with the
    migration to "bun". Consider using "bun" for the build process in the "dev" script
    to maintain consistency across all scripts.** [apps/cli/package.json [10]](https://github.com/keyshade-xyz/keyshade/pull/400/files#diff-1625a7848c01883721bcd146381e2a6bff3c03814d4586559265d8e54a0c9565R10-R10) ```diff -"dev": "pnpm build && node dist/src/index.js" +"dev": "bun build && bun dist/src/index.js" ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion improves consistency across the scripts by using "bun" instead of "pnpm" for building, aligning with the other "bun" scripts.
    		8
    Update the "start" script to use "bun" for execution, aligning with the migration ___ **The "start" script currently uses "node" to execute the JavaScript file. Since the
    migration to "bun" is underway, consider changing this script to use "bun" for
    execution to align with the new environment.** [apps/cli/package.json [9]](https://github.com/keyshade-xyz/keyshade/pull/400/files#diff-1625a7848c01883721bcd146381e2a6bff3c03814d4586559265d8e54a0c9565R9-R9) ```diff -"start": "node dist/src/index.js" +"start": "bun dist/src/index.js" ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 8 Why: This suggestion aligns the "start" script with the ongoing migration to "bun", ensuring consistency in the execution environment.
    		8
    Best practice
    Standardize the output directory flag in the "bun:build" script for consistency ___ **The "bun:build" script specifies "bun build src --outdir dist" which is correct, but
    for consistency and clarity, consider using the same output directory flag as used
    in other tools, "--out-dir" instead of "--outdir".** [apps/cli/package.json [11]](https://github.com/keyshade-xyz/keyshade/pull/400/files#diff-1625a7848c01883721bcd146381e2a6bff3c03814d4586559265d8e54a0c9565R11-R11) ```diff -"bun:build": "bun build src --outdir dist" +"bun:build": "bun build src --out-dir dist" ``` - [ ] **Apply this suggestion**
    Suggestion importance[1-10]: 7 Why: The suggestion promotes consistency in the command-line flags used across different tools, which can help avoid confusion and maintain uniformity.
    		7
    rajdip-b commented 2 months ago

    It's true that the installation will be a pain initially. We will have the docs up for it.

    I think we can't migrate the API to bun because NestJS isnt supported. So, we are mostly aiming for the API Client and CLI to be migrated. And then if possible, we would migrate the frontend.

    RanitMukherjee commented 2 months ago

    It's true that the installation will be a pain initially. We will have the docs up for it.

    I think we can't migrate the API to bun because NestJS isnt supported. So, we are mostly aiming for the API Client and CLI to be migrated. And then if possible, we would migrate the frontend.

    I just checked out Bun Issue in regard to NestJS compatability, and it seems as of 21st June 2024, most of support for NestJS has been established & separate issues regarding edge cases/specific troubleshooting are to be created. Can you clarify in regard to details in regard to specific issues we been encountering, maybe a issue can be raised against the same & once sorted, a full migration to Bun might not be far, thus allowing a simpler set up experience as well.

    rajdip-b commented 2 months ago

    We won't know for sure what exactly the blockers might be, until and unless someone starts working on it. That's why, we have parked it for the very end.

    Jarred-Sumner commented 1 month ago

    let us know if you run into any issues with nestjs in bun

    rajdip-b commented 1 month ago

    @Jarred-Sumner thanks! We are tracking our NestJS migration in #275. We will surely hit you up in case we hit a dead end :D