fix: gracefully fail when node module can't be found#6037
Open
edusperoni wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughPluginsService refactors internal parameter passing and method signatures to use multiline argument formatting for clarity. The key functional change adds null-handling in ChangesPluginsService Refactoring
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/services/plugins-service.ts`:
- Around line 722-737: getNodeModuleData and convertToPluginData can return null
but are typed non-nullable and their callers (notably remove) call
platformProjectService.removePluginNativeCode with that result, which can crash
(e.g., IOSProjectService.removePluginNativeCode dereferences pluginData). Update
getNodeModuleData and convertToPluginData types to allow null/undefined
(INodeModuleData | null, IPluginData | null), and add null guards where their
values are used: in remove(), check the result of
convertToPluginData(getNodeModuleData(...)) before calling
platformProjectService.removePluginNativeCode and skip/log when null; also
update other call sites (like the installed modules mapping) to handle nulls
appropriately (filter/skip or log) so no null is passed into
removePluginNativeCode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 248d5abe-977f-4ec3-853a-82d1dc4a82af
📒 Files selected for processing (1)
lib/services/plugins-service.ts
Comment on lines
722
to
+737
| private getNodeModuleData( | ||
| module: string, | ||
| projectDir: string | ||
| projectDir: string, | ||
| ): INodeModuleData { | ||
| // module can be modulePath or moduleName | ||
| if (!this.$fs.exists(module) || path.basename(module) !== "package.json") { | ||
| module = this.getPackageJsonFilePathForModule(module, projectDir); | ||
| const resolvedPath = this.getPackageJsonFilePathForModule( | ||
| module, | ||
| projectDir, | ||
| ); | ||
| if (!resolvedPath) { | ||
| this.$logger.warn( | ||
| `Could not find module ${color.yellow(module)}. It may have been removed or is not installed. Skipping.`, | ||
| ); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TypeScript nullability settings and all getNodeModuleData call sites.
set -euo pipefail
echo "== tsconfig strict/nullability settings =="
fd -i 'tsconfig*.json' | while read -r f; do
echo "--- $f"
rg -n '"strict"\s*:|"strictNullChecks"\s*:' "$f" || true
done
echo
echo "== getNodeModuleData definition and call sites =="
rg -nP --type=ts -C3 '\bgetNodeModuleData\s*\(' lib/services/plugins-service.ts
echo
echo "== remove flow around removePluginNativeCode =="
rg -nP --type=ts -C5 '\bremovePluginNativeCode\s*\(' lib/services/plugins-service.tsRepository: NativeScript/nativescript-cli
Length of output: 1277
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/services/plugins-service.ts"
echo "== Snippet: getNodeModuleData implementation (around 700-780) =="
sed -n '700,820p' "$file" | nl -ba | sed -n '1,160p'
echo
echo "== Find INodeModuleData type/interface definition =="
rg -n --type=ts '\binterface\s+INodeModuleData\b|\btype\s+INodeModuleData\b' "$file" -S || true
echo
echo "== Search for INodeModuleData usages in the file (limit output) =="
rg -n --type=ts '\bINodeModuleData\b' "$file" | head -n 50
echo
echo "== convertToPluginData signature + return type (and null handling) =="
rg -n --type=ts '\bconvertToPluginData\s*\(' "$file" -n
# show small window around first hit
line=$(rg -n --type=ts '\bconvertToPluginData\s*\(' "$file" | head -n1 | cut -d: -f1)
start=$((line-20))
end=$((line+80))
sed -n "${start},${end}p" "$file" | nl -ba
echo
echo "== removePluginNativeCode call site context (around 120-190) =="
sed -n '120,200p' "$file" | nl -baRepository: NativeScript/nativescript-cli
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="lib/services/plugins-service.ts"
echo "== getNodeModuleData implementation (lines 700-820) =="
sed -n '700,820p' "$file" | cat -n
echo
echo "== Lines in this file that contain getNodeModuleData + return null (limit output) =="
rg -n --type=ts 'getNodeModuleData|return null' "$file" | head -n 200
echo
echo "== convertToPluginData definition + return type/body (limit output) =="
# Find definition line for convertToPluginData, then show a window
def_line=$(rg -n --type=ts 'convertToPluginData\s*\(' "$file" | head -n1 | cut -d: -f1)
start=$((def_line-20))
end=$((def_line+120))
sed -n "${start},${end}p" "$file" | cat -n
echo
echo "== removePluginNativeCode call site context (lines 130-210) =="
sed -n '130,210p' "$file" | cat -n
echo
echo "== Locate INodeModuleData definition/import in repo (not just this file) =="
rg -n --type=ts '\binterface\s+INodeModuleData\b|\btype\s+INodeModuleData\b' . | head -n 50
echo
echo "== Locate convertToPluginData call sites inside this file =="
rg -n --type=ts 'convertToPluginData\s*\(' "$file"Repository: NativeScript/nativescript-cli
Length of output: 12916
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect INodeModuleData definition
defs="lib/definitions/plugins.d.ts"
echo "== INodeModuleData definition in $defs =="
sed -n '1,120p' "$defs" | cat -n
echo
echo "== convertToPluginData definition around line ~622 =="
file="lib/services/plugins-service.ts"
sed -n '580,720p' "$file" | cat -n
echo
echo "== getNodeModuleData return null path context (around ~680-710) =="
sed -n '700,760p' "$file" | cat -n
echo
echo "== Find platformProjectService.removePluginNativeCode signature/type =="
# Locate definition(s) and then show nearby lines
rg -n --type=ts 'removePluginNativeCode\s*\(' . | head -n 50
echo
echo "== Show likely type/interface for removePluginNativeCode (best-effort: matching file names) =="
# try to open the first couple hits with some context
hits=$(rg -n --type=ts 'removePluginNativeCode\s*\(' . | head -n 5 | cut -d: -f1 | sort -u)
for f in $hits; do
echo "--- $f"
sed -n '1,200p' "$f" | head -n 200 | cat -n
doneRepository: NativeScript/nativescript-cli
Length of output: 48646
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig strict/nullability settings values =="
for f in tsconfig.json packages/doctor/tsconfig.json packages/nativescript-envinfo/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n '"strictNullChecks"\s*:\s*(true|false)|"strict"\s*:\s*' "$f" || true
# also show "compilerOptions" block header lines around strict settings
rg -n '"compilerOptions"\s*:\s*{' -n "$f" || true
fi
done
echo
echo "== removePluginNativeCode implementation (android) around declaration =="
rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/android-project-service.ts
lineA=$(rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/android-project-service.ts | head -n1 | cut -d: -f1)
sed -n "$((lineA-40)),$((lineA+120))p" lib/services/android-project-service.ts | cat -n
echo
echo "== removePluginNativeCode implementation (ios) around declaration =="
rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/ios-project-service.ts
lineI=$(rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/ios-project-service.ts | head -n1 | cut -d: -f1)
sed -n "$((lineI-40)),$((lineI+180))p" lib/services/ios-project-service.ts | cat -n
echo
echo "== Other convertToPluginData call sites (context) =="
file="lib/services/plugins-service.ts"
rg -n --type=ts 'convertToPluginData\s*\(' "$file" | while read -r l; do echo "$l"; done
# show contexts around each occurrence (first 6)
for line in $(rg -n --type=ts 'convertToPluginData\s*\(' "$file" | cut -d: -f1 | head -n 6); do
echo "--- context around $line"
sed -n "$((line-15)),$((line+35))p" "$file" | cat -n
doneRepository: NativeScript/nativescript-cli
Length of output: 24048
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tsconfig strict/nullability settings values =="
for f in tsconfig.json packages/doctor/tsconfig.json packages/nativescript-envinfo/tsconfig.json; do
if [ -f "$f" ]; then
echo "--- $f"
rg -n '"strictNullChecks"\s*:\s*(true|false)|"strict"\s*:\s*' "$f" || true
fi
done
echo
echo "== removePluginNativeCode implementation (android) around declaration =="
rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/android-project-service.ts
lineA=$(rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/android-project-service.ts | head -n1 | cut -d: -f1)
sed -n "$((lineA-40)),$((lineA+140))p" lib/services/android-project-service.ts | cat -n
echo
echo "== removePluginNativeCode implementation (ios) around declaration =="
rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/ios-project-service.ts
lineI=$(rg -n --type=ts 'removePluginNativeCode\s*\(' lib/services/ios-project-service.ts | head -n1 | cut -d: -f1)
sed -n "$((lineI-40)),$((lineI+200))p" lib/services/ios-project-service.ts | cat -n
echo
echo "== Other convertToPluginData call sites in plugins-service.ts (context) =="
file="lib/services/plugins-service.ts"
lines=$(rg -n --type=ts 'convertToPluginData\s*\(' "$file" | cut -d: -f1)
for line in $(echo "$lines" | head -n 6); do
echo "--- context around $line"
sed -n "$((line-15)),$((line+40))p" "$file" | cat -n
doneRepository: NativeScript/nativescript-cli
Length of output: 25683
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in tsconfig.json packages/doctor/tsconfig.json packages/nativescript-envinfo/tsconfig.json; do
echo "== $f =="
if [ -f "$f" ]; then
rg -n 'strictNullChecks|"\s*strict"\s*:\s*(true|false)|noImplicitAny' "$f" || true
# show the surrounding compilerOptions block header (if present)
rg -n '"compilerOptions"\s*:' "$f" || true
fi
echo
doneRepository: NativeScript/nativescript-cli
Length of output: 281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in tsconfig.json packages/doctor/tsconfig.json packages/nativescript-envinfo/tsconfig.json; do
echo "== $f (compilerOptions block) =="
# Print first ~120 lines with line numbers for locating strictNullChecks quickly (these files are usually small)
sed -n '1,160p' "$f" | cat -n
echo
doneRepository: NativeScript/nativescript-cli
Length of output: 1038
Fix nullability contract for getNodeModuleData/remove to avoid passing null into removePluginNativeCode.
getNodeModuleData is typed to return INodeModuleData but returns null when the module/package.json can’t be resolved; convertToPluginData is typed to return IPluginData but can also return null on errors. In remove(), the result of convertToPluginData(getNodeModuleData(...)) is passed directly to platformProjectService.removePluginNativeCode without a null guard—IOSProjectService.removePluginNativeCode dereferences pluginData.pluginPlatformsFolderPath(...), so a missing module can crash instead of being skipped.
💡 Proposed fix
- private getNodeModuleData(
+ private getNodeModuleData(
module: string,
projectDir: string,
- ): INodeModuleData {
+ ): INodeModuleData | null { const removePluginNativeCodeAction = async (
modulesDestinationPath: string,
platform: string,
platformData: IPlatformData,
): Promise<void> => {
- const pluginData = this.convertToPluginData(
- this.getNodeModuleData(pluginName, projectData.projectDir),
- projectData.projectDir,
- );
+ const nodeModuleData = this.getNodeModuleData(
+ pluginName,
+ projectData.projectDir,
+ );
+ if (!nodeModuleData) {
+ this.$logger.warn(
+ `Skipping native cleanup for missing module ${color.yellow(pluginName)}.`,
+ );
+ return;
+ }
+ const pluginData = this.convertToPluginData(
+ nodeModuleData,
+ projectData.projectDir,
+ );
+ if (!pluginData) {
+ return;
+ }Also update other getNodeModuleData/convertToPluginData call sites (e.g., the “installed modules” mapping) to align types with the new null behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/services/plugins-service.ts` around lines 722 - 737, getNodeModuleData
and convertToPluginData can return null but are typed non-nullable and their
callers (notably remove) call platformProjectService.removePluginNativeCode with
that result, which can crash (e.g., IOSProjectService.removePluginNativeCode
dereferences pluginData). Update getNodeModuleData and convertToPluginData types
to allow null/undefined (INodeModuleData | null, IPluginData | null), and add
null guards where their values are used: in remove(), check the result of
convertToPluginData(getNodeModuleData(...)) before calling
platformProjectService.removePluginNativeCode and skip/log when null; also
update other call sites (like the installed modules mapping) to handle nulls
appropriately (filter/skip or log) so no null is passed into
removePluginNativeCode.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This solves the annoying error
Summary by CodeRabbit
Bug Fixes
Refactor