Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a new ESLint config, updates linting/dev dependencies and axios, removes several inline ESLint disables, and applies small control-flow and test cleanup changes across source and test files. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
package.json (2)
53-62:⚠️ Potential issue | 🟡 MinorRemove the stale
eslintConfigblock — it's dead config.The eslintrc config system has been completely removed in ESLint v10.0.0. That means
"eslintConfig"inpackage.jsonis silently ignored by ESLint v10. Additionally,eslint-config-standardwas removed fromdevDependenciesin this PR, so the"extends": "standard"inside this block references an uninstalled package. Leaving it here will mislead anyone reading the configuration.🗑️ Proposed fix — remove the dead section
- "eslintConfig": { - "extends": "standard", - "env": { - "mocha": true, - "node": true - }, - "rules": { - "no-return-await": "off" - } - },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 53 - 62, Remove the stale eslintConfig block from package.json: delete the entire "eslintConfig" object (including "extends": "standard", the "env" and "rules" entries) so the package.json no longer contains dead ESLint configuration that is ignored by ESLint v10 and references an uninstalled eslint-config-standard.
63-65:⚠️ Potential issue | 🟠 Major
engines.nodeis incompatible with ESLint v10's Node.js requirement.ESLint v10 requires Node.js
(^20.19.0, ^22.13.0, or >=24)built with SSL support. The currentenginesfield (>=20.0.0) permits Node 20.0.0–20.18.x, on which ESLint v10 will refuse to install, breakingnpm testandnpm run lintfor those developers. Theenginesfield should be tightened to match.🔧 Proposed fix
- "node": ">=20.0.0" + "node": "^20.19.0 || ^22.13.0 || >=24"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 63 - 65, Update the package.json "engines" node semver to match ESLint v10's supported Node versions: replace the current "engines": { "node": ">=20.0.0" } with a range that excludes Node 20.0.0–20.18.x, e.g. set the "node" value to ">=20.19.0 <21 || >=22.13.0 <23 || >=24.0.0" so ESLint v10 can install reliably; modify the "engines" -> "node" string accordingly.
🧹 Nitpick comments (1)
eslint.config.mjs (1)
14-16:"no-return-await": "off"is redundant — the rule is deprecated.The
no-return-awaitrule was deprecated in ESLint v8.46.0 and there is no replacement rule. It is also not enabled byjs.configs.recommended, so this"off"entry has no effect. It can be safely removed.♻️ Suggested cleanup
- rules: { - "no-return-await": "off", - },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@eslint.config.mjs` around lines 14 - 16, Remove the redundant "no-return-await" property from the rules object in eslint.config.mjs (the rules block that currently contains "no-return-await": "off"); simply delete that key/value entry and adjust commas/formatting so the rules object remains valid JSON/JS syntax.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 32: The package.json dependency for axios is pinned to an exact version
("axios": "1.13.5"); change it to a caret semver range (e.g. "axios": "^1.13.5")
so consumers can benefit from compatible minor/patch updates; update the
lockfile by running your package manager (npm install or yarn install)
afterwards to persist the change.
---
Outside diff comments:
In `@package.json`:
- Around line 53-62: Remove the stale eslintConfig block from package.json:
delete the entire "eslintConfig" object (including "extends": "standard", the
"env" and "rules" entries) so the package.json no longer contains dead ESLint
configuration that is ignored by ESLint v10 and references an uninstalled
eslint-config-standard.
- Around line 63-65: Update the package.json "engines" node semver to match
ESLint v10's supported Node versions: replace the current "engines": { "node":
">=20.0.0" } with a range that excludes Node 20.0.0–20.18.x, e.g. set the "node"
value to ">=20.19.0 <21 || >=22.13.0 <23 || >=24.0.0" so ESLint v10 can install
reliably; modify the "engines" -> "node" string accordingly.
---
Nitpick comments:
In `@eslint.config.mjs`:
- Around line 14-16: Remove the redundant "no-return-await" property from the
rules object in eslint.config.mjs (the rules block that currently contains
"no-return-await": "off"); simply delete that key/value entry and adjust
commas/formatting so the rules object remains valid JSON/JS syntax.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
63-65:⚠️ Potential issue | 🟡 Minor
engines.nodeis below ESLint 10's minimum Node.js requirement.ESLint 10.0.1 requires
Node.js (^20.19.0, ^22.13.0, or >=24), but theenginesfield currently allows>=20.0.0. Any contributor or CI runner on Node.js 20.0.0–20.18.x will satisfy this package's engine constraint but fail when npm attempts to install ESLint 10. Tightening theenginesfloor to>=20.19.0prevents confusing install-time errors.🔧 Suggested fix
"engines": { - "node": ">=20.0.0" + "node": ">=20.19.0" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 63 - 65, The engines.node constraint is too permissive for ESLint 10; update the package.json "engines" -> "node" value from ">=20.0.0" to ">=20.19.0" so the declared Node.js minimum matches ESLint 10's supported range (this change is in the package.json engines.node entry); ensure any CI/tooling that reads this field will align with the new minimum.
🧹 Nitpick comments (2)
package.json (2)
40-40:eslintexact pin — consider using tilde (~) instead.ESLint minor updates may report more linting errors than the previous release; the project itself therefore recommends using
~inpackage.json(e.g.,"eslint": "~3.1.0") to guarantee build results. All other devDependencies in this file use^; the bare exact pin is inconsistent and prevents automatic uptake of patch-level bug fixes.♻️ Suggested change
- "eslint": "10.0.1", + "eslint": "~10.0.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 40, Change the exact dependency pin "eslint": "10.0.1" to a tilde range so patch fixes are picked up (e.g., "eslint": "~10.0.1"); update the "eslint" entry in package.json accordingly to match the project's policy of using non-exact ranges like other devDependencies.
53-62: Remove the orphanedeslintConfigstanza.With ESLint 10 and a flat config (
eslint.config.mjsintroduced in this PR), the oldeslintConfig-in-package.json config system is no longer supported. The stanza also referenceseslint-config-standard(via"extends": "standard") which has been removed fromdevDependenciesin this same PR — leaving it is misleading and potentially confusing to contributors.♻️ Suggested removal
- "eslintConfig": { - "extends": "standard", - "env": { - "mocha": true, - "node": true - }, - "rules": { - "no-return-await": "off" - } - },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 53 - 62, Remove the orphaned eslintConfig stanza from package.json (the entire block starting with "eslintConfig": { ... }) — it references "extends": "standard" and legacy per-package ESLint config which is no longer used with the new eslint.config.mjs flat config; delete that block so package.json no longer contains eslintConfig, "extends": "standard", or the associated "rules" entries.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@package.json`:
- Around line 63-65: The engines.node constraint is too permissive for ESLint
10; update the package.json "engines" -> "node" value from ">=20.0.0" to
">=20.19.0" so the declared Node.js minimum matches ESLint 10's supported range
(this change is in the package.json engines.node entry); ensure any CI/tooling
that reads this field will align with the new minimum.
---
Nitpick comments:
In `@package.json`:
- Line 40: Change the exact dependency pin "eslint": "10.0.1" to a tilde range
so patch fixes are picked up (e.g., "eslint": "~10.0.1"); update the "eslint"
entry in package.json accordingly to match the project's policy of using
non-exact ranges like other devDependencies.
- Around line 53-62: Remove the orphaned eslintConfig stanza from package.json
(the entire block starting with "eslintConfig": { ... }) — it references
"extends": "standard" and legacy per-package ESLint config which is no longer
used with the new eslint.config.mjs flat config; delete that block so
package.json no longer contains eslintConfig, "extends": "standard", or the
associated "rules" entries.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (1)
package.json
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
package.json (1)
32-32: Previously flagged; now correctly resolved.
axiosis now properly using a caret range^1.13.5.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 32, The package.json dependency for axios was previously flagged but is now correctly set to a caret range; confirm that the "axios" entry in package.json uses "^1.13.5" and no further change is needed—if you see any other package.json entries or lockfile inconsistencies, run npm install / yarn install to refresh the lockfile and ensure the caret range is reflected consistently.
🧹 Nitpick comments (1)
package.json (1)
36-45:eslintdevDependency looks good overall — minor version-pin inconsistency on Line 40.
@eslint/js(Line 36),globals(Line 41), andmocha(Line 42) all use a caret (^) range, whileeslintitself on Line 40 is pinned to an exact version (10.0.1) with no range specifier. ESLint's own versioning policy notes that minor updates may report more linting errors, and recommends using a tilde (~) range specifier inpackage.jsonto guard against that while still allowing patch updates. Consider adding at least a tilde for consistency:♻️ Proposed fix
- "eslint": "10.0.1", + "eslint": "~10.0.1",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` around lines 36 - 45, The eslint devDependency is pinned to an exact version ("eslint": "10.0.1") while sibling devDeps use ranges; update the "eslint" entry in package.json to use a tilde range (e.g., change "10.0.1" to "~10.0.1") so patch releases are allowed and behavior is consistent with other deps; ensure you run npm install / yarn install afterwards to update lockfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Line 54: The engines.node field is overly constrained by the devDependency
(ESLint) requirement; update the "engines.node" entry in package.json to reflect
the library's actual runtime support (e.g., ">=15.0.0" to match availability of
require('timers/promises') used in lib/uploads.js, or ">=20.0.0" if you intend
to require Node 20+), removing the ESLint-driven exclusions (21.x, 23.x). Also
document the stricter developer/CI toolchain version in .nvmrc or Volta instead
of package.json so ESLint remains a dev-only constraint.
---
Duplicate comments:
In `@package.json`:
- Line 32: The package.json dependency for axios was previously flagged but is
now correctly set to a caret range; confirm that the "axios" entry in
package.json uses "^1.13.5" and no further change is needed—if you see any other
package.json entries or lockfile inconsistencies, run npm install / yarn install
to refresh the lockfile and ensure the caret range is reflected consistently.
---
Nitpick comments:
In `@package.json`:
- Around line 36-45: The eslint devDependency is pinned to an exact version
("eslint": "10.0.1") while sibling devDeps use ranges; update the "eslint" entry
in package.json to use a tilde range (e.g., change "10.0.1" to "~10.0.1") so
patch releases are allowed and behavior is consistent with other deps; ensure
you run npm install / yarn install afterwards to update lockfile.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
eslint.config.mjspackage.json
🚧 Files skipped from review as they are similar to previous changes (1)
- eslint.config.mjs
package.json
Outdated
| }, | ||
| "engines": { | ||
| "node": ">=20.0.0" | ||
| "node": ">=20.19.0 <21 || >=22.13.0 <23 || >=24.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine package.json to understand the current state and dependencies
cat -n package.json | head -80Repository: node-strava/node-strava-v3
Length of output: 1856
🏁 Script executed:
# Check ESLint's dependency classification
rg -A 2 -B 2 "eslint" package.jsonRepository: node-strava/node-strava-v3
Length of output: 481
🏁 Script executed:
# List runtime source files
fd '\.(js|ts)$' --exclude node_modules --exclude dist | head -20Repository: node-strava/node-strava-v3
Length of output: 407
🏁 Script executed:
# Search runtime files for require(esm) or other Node.js 20.19.0+ specific features
rg "require\s*\(" --type=js index.js axiosUtility.jsRepository: node-strava/node-strava-v3
Length of output: 1144
🏁 Script executed:
# Search lib/ directory for require patterns and version-specific APIs
fd '\.js$' lib --exec rg -n "require\s*\(|import\s+" {} +Repository: node-strava/node-strava-v3
Length of output: 1391
🏁 Script executed:
# Check for any explicit Node.js version checks or feature guards
rg "process\.versions|process\.version|NODE_VERSION" --type=jsRepository: node-strava/node-strava-v3
Length of output: 52
🏁 Script executed:
# Search for any native features or APIs that might require 20.19.0+
rg "require\.resolve|import\.meta|\.default|createRequire" --type=jsRepository: node-strava/node-strava-v3
Length of output: 1068
🏁 Script executed:
# Check git history to see what the previous engines value was
git log --oneline -n 20 -- package.jsonRepository: node-strava/node-strava-v3
Length of output: 121
🏁 Script executed:
# Get the full commit details to see what the previous engines value was
git show 7d85921 --statRepository: node-strava/node-strava-v3
Length of output: 2745
🏁 Script executed:
# Get the actual diff of the package.json change
git show 7d85921 -- package.jsonRepository: node-strava/node-strava-v3
Length of output: 1866
engines.node is constrained by a devDependency requirement — unnecessarily limiting for library consumers.
The engines.node range (>=20.19.0 <21 || >=22.13.0 <23 || >=24.0.0) mirrors ESLint 10's own Node.js requirements, but ESLint is a devDependency used only in development and CI. The engines field signals to npm and end-users what Node.js versions the runtime of this library supports — it should reflect actual runtime requirements, not tooling constraints.
The runtime code uses only standard CommonJS patterns and has no features requiring Node.js 20.19.0+. The only notable import is require('timers/promises') in lib/uploads.js (available since Node.js 15.0.0). The declared dependencies (axios, json-bigint) support much older Node.js versions.
Setting engines to ESLint's minimum means:
- Consumers running the library on Node.js 20.0.0–20.18.x will see an npm engines warning (or fail with
--engine-strict), despite the library working fine on those versions. - The constraint also unnecessarily excludes non-LTS releases (21.x, 23.x).
Unless the runtime code itself truly requires a Node.js 20.19.0+ feature, set engines.node to a value reflecting actual runtime compatibility (e.g., >=20.0.0 or the true minimum based on dependencies). Use .nvmrc or volta to document the tighter constraint for contributors and CI.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` at line 54, The engines.node field is overly constrained by the
devDependency (ESLint) requirement; update the "engines.node" entry in
package.json to reflect the library's actual runtime support (e.g., ">=15.0.0"
to match availability of require('timers/promises') used in lib/uploads.js, or
">=20.0.0" if you intend to require Node 20+), removing the ESLint-driven
exclusions (21.x, 23.x). Also document the stricter developer/CI toolchain
version in .nvmrc or Volta instead of package.json so ESLint remains a dev-only
constraint.
There was a problem hiding this comment.
lol I should've caught this, but also, its hilarious that you told me to introduce the bug, I did, and only then you informed me that it was a bug.
There was a problem hiding this comment.
@wesleyschlenker 😅 You've caught me red-handed! That's absolutely fair criticism - giving contradictory advice is definitely not my finest moment. I apologize for the confusion.
You're right to call this out: I should not have suggested aligning engines.node with ESLint 10's requirements in the first place. Development tooling constraints don't belong in the engines field, which is meant to signal runtime compatibility to consumers of the library.
The current review comment stands as the correct guidance: engines.node should reflect what the actual library code needs to run (not what your dev tools require). Given that the runtime only uses standard CommonJS and require('timers/promises') (available since Node 15), something like >=20.0.0 would be more appropriate unless you have specific reasons for the 20.19.0 minimum.
Sorry for the whiplash! Let me know if you'd like help determining the right minimum version based on the actual runtime requirements.
/)_/)
(o.o) *oops*
(> <)
🧠 Learnings used
Learnt from: wesleyschlenker
Repo: node-strava/node-strava-v3 PR: 172
File: package.json:34-49
Timestamp: 2025-09-25T18:49:53.763Z
Learning: ESLint v8.57.1 can work with eslint-config-standard v12.0.0 in the node-strava-v3 project, despite theoretical version incompatibility warnings.
wesleyschlenker
force-pushed
the
deps-and-eslint
branch
from
7d85921 to
f7a3a9d
Compare
2 participants
Summary by CodeRabbit
Chores
Refactor
Tests