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

Improve Frontend Makefile run command and Backend setup #50

Merged
merged 4 commits into from
Nov 4, 2024

Conversation

0xrohitgarg
Copy link
Collaborator

@0xrohitgarg 0xrohitgarg commented Nov 4, 2024

Pull Request Summary

This PR introduces improvements to both the Frontend Makefile run command and Backend Setup to enhance usability, flexibility, and ensure compatibility with project requirements.

1. Frontend Makefile: make run-fe Command Enhancements

The make run-fe command logic has been updated to provide users with greater flexibility regarding Node.js versions, particularly when using nvm.

Logic Updates:

  • When nvm is installed:

    • If NODE_VERSION (the LTS Node version set in setup.sh) is installed via nvm, use that version.
    • If NODE_VERSION is not installed but a version is available that exceeds MIN_NODE_VERSION (the minimum Node version supported by this project), use that compatible version.
    • If no compatible version is found, install NODE_VERSION via nvm and use it.
  • When nvm is not installed:

    • The user's default Node version is used.

Note

  • NODE_VERSION refers to the specific Node.js version in LTS, defined in setup.sh.
  • MIN_NODE_VERSION is the minimum Node.js version that this project supports.

2. Backend Setup Enhancement

  • The setup.sh script now includes an update for pip to ensure the latest version is installed, improving compatibility and security for Python package installations.

@ashish-spext
Copy link
Contributor

Failing Scenario: If the user has nvm installed but hasn't installed Node.js version 22.8.0 through setup.sh and prefers to use their current version.

Questions:

  • What is the minimum Node.js version supported by our frontend?
  • Is there any specific advantage to pinning the version to 22.8.0?

Exploration:

  • Can we modify the command to allow users to either use version 22.8.0 or any version greater than the minimum supported version, to offer more flexibility?

@ashish-spext
Copy link
Contributor

Also, if we can have pip --upgrade pip in setup.sh as part of this PR only that would be great.

- If nvm is installed
   - If reqired version is installed with nvm; use that
   - If required version is not present, then check versions above minimum node version; use that
   - If no version is found, install the required version with nvm and use that
- If nvm is not installed
   - Use user's default node version
:
@0xrohitgarg
Copy link
Collaborator Author

Updated the makefile run command to run the following logic,

Two node version are mentioned in the script :

  • required version is the version that project installed during setup.sh which is current lts
  • minimum version is the minimum version required to run the project

Updated Logic

  • If nvm is installed
    • If required version is installed with nvm; use that
    • If required version is not present, then check versions above minimum version; use that
    • If no version is found, install the required version with nvm and use that
  • If nvm is not installed
    • Use user's default node version

Copy link
Contributor

@ashish-spext ashish-spext left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All 3 cases are working.

  1. Required version is available (use it)
  2. > min required version is available (use it)
  3. < min required version is available (install required version and use it)
    Note: If nvm is installed it will have atleast one version so there won't be a case for no version is installed.

I think 3rd case might be super rare but it is good that it is covered.
There can be one more possibility in case 3, that npm install is not done. In that case user will get error vite: not found and they should be able to take care of installation.

Let's merge it after updating the title.

@0xrohitgarg 0xrohitgarg changed the title switch node version on make run Improve Frontend Makefile run command and Backend setup Nov 4, 2024
@0xrohitgarg 0xrohitgarg merged commit 80b4c34 into main Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants