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: Enforce pnpm as the package manager in package.json (fixes #366) #467

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

itsaryan72
Copy link

@itsaryan72 itsaryan72 commented Sep 25, 2024

User description

GENERAL: Only allow PNPM #366

#description
Restriction on Package Manager Usage
To ensure consistency in our project, I've added a preinstall script in package.json.
"preinstall": "if [[ "$npm_config_user_agent" != pnpm ]]; then echo 'This project uses pnpm. Please run using pnpm.'; exit 1; fi",

This script checks the package manager before installation and displays an error if anything other than PNPM is used, promoting uniformity among contributors.

Fixes #366
GENERAL: Only allow PNPM #366

Dependencies

No additional packages or dependencies are required for the preinstall script

Developer's checklist

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

If changes are made in the code:

  • [Y] I have followed the coding guidelines
  • [Y] My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • [Y] There are no UI/UX issues

Documentation Update

  • This PR requires an update to the documentation at docs.keyshade.xyz
  • I have made the necessary updates to the documentation, or no documentation changes are required.

PR Type

enhancement, configuration changes


Description

  • Added a preinstall script in package.json to enforce the use of PNPM as the package manager.
  • The script checks the npm_config_user_agent and exits with an error message if a package manager other than PNPM is used.
  • This change ensures consistency and uniformity among contributors by restricting package manager usage.

Changes walkthrough 📝

Relevant files
Configuration changes
package.json
Enforce PNPM as the package manager in preinstall script 

package.json

  • Added a preinstall script to enforce the use of PNPM.
  • The script checks the package manager and exits with an error if not
    PNPM.
  • +1/-0     

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Compatibility
    The preinstall script may not work correctly on Windows systems due to the use of Bash-specific syntax.

    @itsaryan72 itsaryan72 changed the title modified package.json to add only pnpm package manager (GENERAL: Only allow PNPM #366) modified package.json to add only pnpm package manager (Issue #366) Sep 25, 2024
    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Improve the accuracy of the package manager check in the preinstall script

    Consider using a more robust check for PNPM by explicitly checking for 'pnpm/' at
    the start of the user agent string. This will prevent false positives if 'pnpm'
    appears elsewhere in the user agent string.

    package.json [93]

    -"preinstall": "if [[ \"$npm_config_user_agent\" != *pnpm* ]]; then echo 'This project uses pnpm. Please run using pnpm.'; exit 1; fi",
    +"preinstall": "if [[ \"$npm_config_user_agent\" != pnpm/* ]]; then echo 'This project uses pnpm. Please run using pnpm.'; exit 1; fi",
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion improves the robustness of the check by ensuring that 'pnpm/' is at the start of the user agent string, reducing the chance of false positives.

    8
    User experience
    Provide more helpful information in the error message of the preinstall script

    Consider adding a more informative error message that includes instructions on how
    to install PNPM if it's not already installed.

    package.json [93]

    -"preinstall": "if [[ \"$npm_config_user_agent\" != *pnpm* ]]; then echo 'This project uses pnpm. Please run using pnpm.'; exit 1; fi",
    +"preinstall": "if [[ \"$npm_config_user_agent\" != *pnpm* ]]; then echo 'This project uses pnpm. Please run using pnpm. To install pnpm, visit https://pnpm.io/installation'; exit 1; fi",
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding instructions on how to install PNPM enhances user experience by providing immediate guidance, though it is not crucial for functionality.

    7

    💡 Need additional feedback ? start a PR chat

    @itsaryan72 itsaryan72 changed the title modified package.json to add only pnpm package manager (Issue #366) modified package.json for pnpm manager ( Solving Issue #366) Sep 25, 2024
    @itsaryan72 itsaryan72 changed the title modified package.json for pnpm manager ( Solving Issue #366) build:modified package.json for pnpm manager ( Solving Issue #366) Sep 25, 2024
    @itsaryan72 itsaryan72 changed the title build:modified package.json for pnpm manager ( Solving Issue #366) build: enforce pnpm as the package manager in package.json (fixes #366) Sep 25, 2024
    @rajdip-b
    Copy link
    Member

    @kriptonian1

    @rajdip-b rajdip-b changed the title build: enforce pnpm as the package manager in package.json (fixes #366) chore: Enforce pnpm as the package manager in package.json (fixes #366) Sep 30, 2024
    @kriptonian1
    Copy link
    Contributor

    Hey @itsaryan72, it is returning Unsupported Protocol error than returning the intended messages, can you look into this
    image

    @itsaryan72
    Copy link
    Author

    itsaryan72 commented Sep 30, 2024

    Hello @kriptonian1 , Thanks for reviewing my work. I think the npm package depends on some other dependencies. In case of "yarn install" it gives the correct error "Error: Only pnpm is allowed. Please use pnpm instead.".

    Tried removing all the workspace keyword and file but still it gives the same error. I seriously want to solve this error kindly help me.

    @kriptonian1
    Copy link
    Contributor

    Buddy, I checked it's still not working

    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.

    GENERAL: Only allow PNPM
    4 participants