fix: local setup issues #576
Conversation
|
@hsnice16 is attempting to deploy a commit to the Antiwork Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe development script now enforces specific Node.js and PNPM versions by reading and comparing them before proceeding. If the versions do not match the required ones, the script exits with an error. SSL certificate generation is moved to occur only after successful version checks and dependency installation, and only if the certificate does not already exist. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
|
Looks like you have a very old PNPM version - please try using 10.8.0 as specified in package.json
Please check you're on Node 22 as well Either of those might be related - I haven't seen these problems before 🤔 |
|
+1. Was facing similar issues in my other laptop with node version = 18. Upgrading the node version will do the job here. |
hsnice16
force-pushed
the
fix/local-setup-issues
branch
from
0a8db2d to
13ae364
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.pnpm-version(1 hunks)README.md(1 hunks)scripts/dev.sh(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- .pnpm-version
- README.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (1)
scripts/dev.sh (1)
29-31: 👍 Conditional SSL generation moved to post-installRunning certificate generation only when missing—and after dependencies are in place—prevents needless rebuilds on every
dev.shinvocation. Looks good.
| # Node.js version check | ||
| current_node_version=$(node -v 2>/dev/null || echo "not found") | ||
| if [ "$current_node_version" != "$REQUIRED_NODE_VERSION" ]; then | ||
| echo -e "\033[31m✖ Required Node.js version is $REQUIRED_NODE_VERSION, but found $current_node_version.\033[0m" | ||
| echo "Please install the correct version using nvm or your preferred version manager." | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
String equality check is too strict for patch updates
Using != against the full node -v output rejects harmless patch bumps (e.g. v22.14.1). A simple semver-aware comparison is safer.
If you want zero dependencies:
IFS=. read -r major minor _ <<<"${current_node_version#v}"
if [[ $major -ne 22 || $minor -lt 14 ]]; then
# ...
fiOr rely on semver (already in most dev boxes):
if ! semver -r "22.x" "$current_node_version"; then … ; fiThis prevents unnecessary blocking while still enforcing the major/minor track.
🤖 Prompt for AI Agents
In scripts/dev.sh around lines 17 to 23, the current Node.js version check uses
strict string equality which blocks harmless patch updates. Replace the string
equality check with a semver-aware comparison by parsing the major and minor
version numbers from current_node_version and comparing them numerically to the
required major and minor versions. Alternatively, use a semver tool if available
to check version compatibility. This ensures only major or minor version
mismatches cause an error, allowing patch updates to pass.
scripts/dev.sh
Outdated
| fi | ||
|
|
||
| corepack enable | ||
| corepack prepare |
There was a problem hiding this comment.
corepack prepare does nothing without a target version
Without arguments Corepack silently exits, so the intended pnpm pin never happens. Point it at the version declared in .pnpm-version (added in this PR) and activate it in one go:
-corepack prepare
+corepack prepare "pnpm@$(cat .pnpm-version)" --activateThis guarantees every contributor ends up on the same pnpm binary that matches CI.
🤖 Prompt for AI Agents
In scripts/dev.sh at line 26, the command 'corepack prepare' is used without
specifying a target version, causing it to silently exit and not pin the
intended pnpm version. Modify the command to include the version from the
.pnpm-version file by reading that version and passing it as an argument to
'corepack prepare', ensuring the correct pnpm version is activated for all
contributors.
scripts/dev.sh
Outdated
| REQUIRED_NODE_VERSION="v22.14.0" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid scattering the single source of truth for the Node.js version
The version string is hard-coded here while most contributors will look for a .nvmrc or an engines.node entry in package.json. Divergence is almost guaranteed the next time Node is bumped.
-REQUIRED_NODE_VERSION="v22.14.0"
+# Prefer centralising the version in .nvmrc (already committed by convention)
+if [[ -f .nvmrc ]]; then
+ REQUIRED_NODE_VERSION="v$(cat .nvmrc)"
+else
+ REQUIRED_NODE_VERSION="v22.14.0" # fallback
+fiThis keeps the script in lock-step with common tooling (nvm, volta, asdf, CI images…).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| REQUIRED_NODE_VERSION="v22.14.0" | |
| # Prefer centralising the version in .nvmrc (already committed by convention) | |
| if [[ -f .nvmrc ]]; then | |
| REQUIRED_NODE_VERSION="v$(cat .nvmrc)" | |
| else | |
| REQUIRED_NODE_VERSION="v22.14.0" # fallback | |
| fi |
🤖 Prompt for AI Agents
In scripts/dev.sh around lines 5 to 6, the Node.js version is hard-coded as a
string, which risks divergence from the version specified in .nvmrc or
package.json. Modify the script to dynamically read the Node.js version from the
.nvmrc file or the engines.node field in package.json instead of using a
hard-coded string, ensuring consistency with common tooling and reducing
maintenance overhead.
|
Yeah, the PNPM version was the issue. (I had already updated the node version) @binary-koan I have updated the README to explicitly include that, have added the node version check in the dev.sh, and have also added the missing |
.pnpm-version
Outdated
| @@ -0,0 +1 @@ | |||
| 10.8.0 | |||
There was a problem hiding this comment.
This isn't a standard file naming convention is it? If it's not used by other tools I think it should be inline in the dev script
There was a problem hiding this comment.
I created this to refer it in the README.md.
There was a problem hiding this comment.
And, I am also using it in the check for pnpm version in the dev script.
There was a problem hiding this comment.
I think it's misleading though because if you're used to .node-version, .ruby-version etc then it looks like it's meant to be read by a tool like nodenv or rbenv. Since I'd rather remove the line from the README anyway, let's hardcode this in dev.sh
README.md
Outdated
| @@ -26,6 +26,7 @@ You'll need: | |||
|
|
|||
| - [Docker](https://docs.docker.com/get-docker/) | |||
| - [Node.js](https://nodejs.org/en/download/) (see [`.node-version`](.node-version)) | |||
| - [pnpm](https://pnpm.io/installation) (see [`.pnpm-version`](.pnpm-version)) | |||
There was a problem hiding this comment.
Corepack means you don't need to install pnpm manually, doesn't it?
There was a problem hiding this comment.
Yes. I mentioned this explicitly so the devs can install it before the dev script command. So, they do not face any issues whatsoever.
There was a problem hiding this comment.
But we can decide if we want to remove it.
There was a problem hiding this comment.
Let's remove it to keep things simple, we want as much of setup to be automated as possible
scripts/dev.sh
Outdated
| @@ -2,6 +2,8 @@ | |||
|
|
|||
| set -e | |||
|
|
|||
| REQUIRED_NODE_VERSION="v22.14.0" | |||
There was a problem hiding this comment.
We should read this from .node-version - fewer places to change when we upgrade Node
There was a problem hiding this comment.
I have made this change.
scripts/dev.sh
Outdated
| fi | ||
|
|
||
| corepack enable | ||
| corepack prepare |
There was a problem hiding this comment.
This command seems to be deprecated. Does corepack enable not switch to the correct version?
There was a problem hiding this comment.
It is. But earlier, I was facing an issue with the pnpm version (the reason for this PR :)), so I explicitly added this.
There was a problem hiding this comment.
I think the pnpm version check is enough, enable is supposed to activate the correct version on its own (we can reconsider if more people run into the issue)
There was a problem hiding this comment.
Okay. I have reverted this change.
| # Node.js version check | ||
| current_node_version=$(node -v 2>/dev/null || echo "not found") | ||
| if [ "$current_node_version" != "$REQUIRED_NODE_VERSION" ]; then | ||
| echo -e "\033[31m✖ Required Node.js version is $REQUIRED_NODE_VERSION, but found $current_node_version.\033[0m" |
There was a problem hiding this comment.
Good idea - how about adding a similar check for pnpm version below?
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
scripts/dev.sh (2)
18-24: Node version gate is still an exact-string compare (previously flagged)
String equality rejects harmless patch bumps such asv22.14.1. Please switch to a semver-aware check instead of a literal compare.-current_node_version=$(node -v 2>/dev/null || echo "not found") -if [ "$current_node_version" != "$REQUIRED_NODE_VERSION" ]; then - echo -e "\033[31m✖ Required Node.js version is $REQUIRED_NODE_VERSION, but found $current_node_version.\033[0m" +current_node_version=$(node -v 2>/dev/null || echo "not found") +IFS=. read -r cur_major cur_minor _ <<<"${current_node_version#v}" +IFS=. read -r req_major req_minor _ <<<"${REQUIRED_NODE_VERSION#v}" +if [[ $cur_major -ne $req_major || $cur_minor -lt $req_minor ]]; then + echo -e "\033[31m✖ Required Node.js ≥ $REQUIRED_NODE_VERSION, but found $current_node_version.\033[0m"
26-28:corepack preparestill invoked without a target version
Runningcorepack preparebare is a no-op (it exits 0 and changes nothing). Pin to the version in.pnpm-versionand activate in one shot, e.g.:-corepack prepare +corepack prepare "pnpm@${REQUIRED_PNPM_VERSION}" --activate
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/dev.sh(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cursor BugBot
🔇 Additional comments (1)
scripts/dev.sh (1)
38-41: Certificate generation race-condition
pnpm generate-ssl-certificatesrelies on pnpm; ensure it comes after the pnpm version gate (see comment above) to prevent failures when the wrong pnpm is on PATH.
| # PNPM version check | ||
| current_pnpm_version=$(pnpm -v 2>/dev/null || echo "not found") | ||
| if [ "$current_pnpm_version" != "$REQUIRED_PNPM_VERSION" ]; then | ||
| echo -e "\033[31m✖ Required PNPM version is $REQUIRED_PNPM_VERSION, but found $current_pnpm_version.\033[0m" | ||
| echo "Please install the correct version using: corepack prepare pnpm@$REQUIRED_PNPM_VERSION --activate" | ||
| exit 1 | ||
| fi | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exact match on pnpm version is brittle
As with Node, an equality test blocks patch-level updates. Use cut -d. or semver to allow 10.8.x.
-current_pnpm_version=$(pnpm -v 2>/dev/null || echo "not found")
-if [ "$current_pnpm_version" != "$REQUIRED_PNPM_VERSION" ]; then
+current_pnpm_version=$(pnpm -v 2>/dev/null || echo "not found")
+IFS=. read -r cur_major cur_minor _ <<<"$current_pnpm_version"
+IFS=. read -r req_major req_minor _ <<<"$REQUIRED_PNPM_VERSION"
+if [[ $cur_major -ne $req_major || $cur_minor -lt $req_minor ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # PNPM version check | |
| current_pnpm_version=$(pnpm -v 2>/dev/null || echo "not found") | |
| if [ "$current_pnpm_version" != "$REQUIRED_PNPM_VERSION" ]; then | |
| echo -e "\033[31m✖ Required PNPM version is $REQUIRED_PNPM_VERSION, but found $current_pnpm_version.\033[0m" | |
| echo "Please install the correct version using: corepack prepare pnpm@$REQUIRED_PNPM_VERSION --activate" | |
| exit 1 | |
| fi | |
| # PNPM version check | |
| current_pnpm_version=$(pnpm -v 2>/dev/null || echo "not found") | |
| IFS=. read -r cur_major cur_minor _ <<<"$current_pnpm_version" | |
| IFS=. read -r req_major req_minor _ <<<"$REQUIRED_PNPM_VERSION" | |
| if [[ $cur_major -ne $req_major || $cur_minor -lt $req_minor ]]; then | |
| echo -e "\033[31m✖ Required PNPM version is $REQUIRED_PNPM_VERSION, but found $current_pnpm_version.\033[0m" | |
| echo "Please install the correct version using: corepack prepare pnpm@$REQUIRED_PNPM_VERSION --activate" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In scripts/dev.sh around lines 30 to 37, the current pnpm version check uses
exact string equality, which blocks patch-level updates. Modify the version
check to allow any patch version within the required minor version by parsing
the version strings with a tool like cut or semver. For example, extract the
major and minor parts of both current and required versions and compare those
instead of the full version string to permit patch updates.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
scripts/dev.sh
Outdated
| fi | ||
|
|
||
| corepack enable | ||
| corepack prepare |
There was a problem hiding this comment.
I think the pnpm version check is enough, enable is supposed to activate the correct version on its own (we can reconsider if more people run into the issue)
README.md
Outdated
| @@ -26,6 +26,7 @@ You'll need: | |||
|
|
|||
| - [Docker](https://docs.docker.com/get-docker/) | |||
| - [Node.js](https://nodejs.org/en/download/) (see [`.node-version`](.node-version)) | |||
| - [pnpm](https://pnpm.io/installation) (see [`.pnpm-version`](.pnpm-version)) | |||
There was a problem hiding this comment.
Let's remove it to keep things simple, we want as much of setup to be automated as possible
.pnpm-version
Outdated
| @@ -0,0 +1 @@ | |||
| 10.8.0 | |||
There was a problem hiding this comment.
I think it's misleading though because if you're used to .node-version, .ruby-version etc then it looks like it's meant to be read by a tool like nodenv or rbenv. Since I'd rather remove the line from the README anyway, let's hardcode this in dev.sh
There was a problem hiding this comment.
Bug: Node Version Check Fails with Missing or Prefixed Files
The dev.sh script's Node.js version check is flawed. If the .node-version file is missing, cat .node-version fails, causing REQUIRED_NODE_VERSION to be incorrectly set to "v". This leads to a misleading error message (e.g., "Required Node.js version is v, but found vX.Y.Z"). Additionally, if the .node-version file already contains a "v" prefix (e.g., "v20.0.0"), the script prepends another "v", resulting in "vv20.0.0" and causing the version comparison to fail.
scripts/dev.sh#L4-L5
Lines 4 to 5 in 9d7b630
Was this report helpful? Give feedback by reacting with 👍 or 👎
Follow-up to #576 On second thoughts, we don't need to be too prescriptive about the exact Node version for people without a version manager. We can still match exactly for pnpm since it's installed with corepack.
|
Thanks for merging it 👍 |
|
By any chance, does this PR come in the @slavingia post? https://x.com/shl/status/1935719392630698461 |
|
Nope sorry! |
Closes the discussion antiwork#555 **Context:** I was facing multiple issues while doing the local setup of the projects, which I have mentioned in the discussion antiwork#555. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Chores** - Improved version checks for Node.js and PNPM, with clear error messages if mismatched. - SSL certificate generation is now conditional and occurs after dependency installation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Follow-up to antiwork#576 On second thoughts, we don't need to be too prescriptive about the exact Node version for people without a version manager. We can still match exactly for pnpm since it's installed with corepack.
Closes the discussion #555
Context:
I was facing multiple issues while doing the local setup of the projects, which I have mentioned in the discussion #555.
Summary by CodeRabbit