feat: enable per-platform language selection and match with new nitro version#470
Merged
patrickkabwe merged 8 commits intopatrickkabwe:mainfrom Mar 16, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors language configuration from a flat Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as CLI
participant Selector as selectPlatformLanguages()
participant Map as PlatformLangMap
participant Factory as NitroModuleFactory
participant AndroidGen as AndroidFileGenerator
participant IOSGen as IOSFileGenerator
participant CppGen as CppFileGenerator
participant JSGen as JSFileGenerator
participant Utils as generateAutolinking()
participant Output as Generated_Files
User->>CLI: provide preferences / flags
CLI->>Selector: selectPlatformLanguages()
Selector->>Map: build PlatformLangMap
CLI->>Factory: pass config with platformLangs
Factory->>Map: read platformLangs
alt all platforms == CPP
Factory->>CppGen: generate composite CPP module
else
Factory->>CppGen: generateCppCodeFiles (if CPP present)
Factory->>AndroidGen: generate() when Map[ANDROID] set
Factory->>IOSGen: generate() when Map[IOS] set
end
Factory->>JSGen: generate() using Map entries
JSGen->>Utils: generateAutolinking(moduleName, platformLangs)
JSGen->>Output: write JS & autolinking files
Factory->>Output: finalize package and run post-codegen based on platformLangs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 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)
📝 Coding Plan
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.
🧹 Nitpick comments (5)
src/file-generators/js-file-generator.ts (1)
17-22: Minor redundancy in CPP mapping.The check
lang === SupportedLang.CPP ? 'c++' : langis redundant sinceSupportedLang.CPPis already defined as'c++'intypes.ts. However, this is harmless and maintains consistency with thelangToNitroLangpattern inutils.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/file-generators/js-file-generator.ts` around lines 17 - 22, The mapping that converts config.platformLangs to platformLang redundantly checks lang === SupportedLang.CPP to map to 'c++' even though SupportedLang.CPP is already 'c++'; simplify the expression in src/file-generators/js-file-generator.ts by removing the conditional and using the lang value directly (i.e., map ([platform, lang]) => `${platform}: '${lang}'`) while keeping the surrounding logic intact to match the pattern used in utils.ts and types.ts (refer to platformLang, config.platformLangs, and SupportedLang.CPP).src/generate-nitro-package.ts (1)
199-204: Duplication of Kotlin check logic.The Kotlin check
Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN)is duplicated betweenupdatePackageJsonConfig(line 199-201) andinstallDependenciesAndRunCodegen(line 524). Consider extracting this to a helper method or property for consistency and maintainability.♻️ Suggested refactor
Add a private helper method:
private usesKotlin(): boolean { return Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN) }Then use it in both locations:
- codegen: `nitrogen --logLevel="debug" && ${this.config.pm} run build${Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN) ? ' && node post-script.js' : ''}`, - postcodegen: !Object.values(this.config.platformLangs).includes( - SupportedLang.KOTLIN - ) + codegen: `nitrogen --logLevel="debug" && ${this.config.pm} run build${this.usesKotlin() ? ' && node post-script.js' : ''}`, + postcodegen: !this.usesKotlin() ? this.getPostCodegenScript() : undefined,And in
installDependenciesAndRunCodegen:- let codegenCommand = `${packageManager} nitrogen --logLevel="debug" && ${this.config.pm} run build${Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN) ? ' && node post-script.js' : ''}` + let codegenCommand = `${packageManager} nitrogen --logLevel="debug" && ${this.config.pm} run build${this.usesKotlin() ? ' && node post-script.js' : ''}`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generate-nitro-package.ts` around lines 199 - 204, The Kotlin presence check is duplicated; add a private helper on the class (e.g., private usesKotlin(): boolean) that returns Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN), then replace the duplicated expressions in updatePackageJsonConfig (the codegen/postcodegen conditional) and installDependenciesAndRunCodegen with calls to usesKotlin() to centralize the logic and improve maintainability.src/cli/create.ts (1)
194-200: Minor duplication between CI and View language resolution.In CI mode, the module case (lines 197-200) manually constructs the same mapping that
resolveViewLanguageswould return. Consider reusingresolveViewLanguagesfor consistency:♻️ Suggested refactor
platformLangs: packageType === Nitro.View ? resolveViewLanguages(platforms) - : { - [SupportedPlatform.IOS]: SupportedLang.SWIFT, - [SupportedPlatform.ANDROID]: SupportedLang.KOTLIN, - }, + : resolveViewLanguages(platforms),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/create.ts` around lines 194 - 200, The platformLangs assignment duplicates logic: when packageType === Nitro.View the code calls resolveViewLanguages(platforms) but in the CI branch it manually builds the same mapping; replace the manual mapping with a call to resolveViewLanguages(platforms) so both branches reuse the same resolver. Update the platformLangs expression to call resolveViewLanguages(platforms) for the CI/module case instead of hardcoding { [SupportedPlatform.IOS]: SupportedLang.SWIFT, [SupportedPlatform.ANDROID]: SupportedLang.KOTLIN }, referencing the existing resolveViewLanguages, packageType, Nitro.View, and platforms symbols.src/utils.ts (1)
120-122: Remove unusedvalidateTemplatefunction.The
validateTemplatefunction is not referenced anywhere in the codebase and appears to be a leftover from a previous implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 120 - 122, Remove the unused exported function validateTemplate from src/utils.ts: delete the export const validateTemplate = (answer: string[]) => ... declaration, and then run a project-wide search for "validateTemplate" to remove any stale imports/usages (or update them if found) and adjust exports if this was the only export in the module so module consumers still import valid symbols.src/types.ts (1)
27-30: Remove the unusedPlatformLangtype definition.The
PlatformLangtype is not used anywhere in the codebase. The codebase usesPlatformLangMapinstead, which is the correct type for the current design. This type is a remnant from the old design and should be removed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 27 - 30, Remove the unused PlatformLang type definition: locate the exported type PlatformLang in src/types.ts and delete its declaration so the codebase only exposes PlatformLangMap (the correct type). Ensure there are no remaining references to PlatformLang; if any tests or imports reference it, update them to use PlatformLangMap or the appropriate type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/create.ts`:
- Around line 194-200: The platformLangs assignment duplicates logic: when
packageType === Nitro.View the code calls resolveViewLanguages(platforms) but in
the CI branch it manually builds the same mapping; replace the manual mapping
with a call to resolveViewLanguages(platforms) so both branches reuse the same
resolver. Update the platformLangs expression to call
resolveViewLanguages(platforms) for the CI/module case instead of hardcoding {
[SupportedPlatform.IOS]: SupportedLang.SWIFT, [SupportedPlatform.ANDROID]:
SupportedLang.KOTLIN }, referencing the existing resolveViewLanguages,
packageType, Nitro.View, and platforms symbols.
In `@src/file-generators/js-file-generator.ts`:
- Around line 17-22: The mapping that converts config.platformLangs to
platformLang redundantly checks lang === SupportedLang.CPP to map to 'c++' even
though SupportedLang.CPP is already 'c++'; simplify the expression in
src/file-generators/js-file-generator.ts by removing the conditional and using
the lang value directly (i.e., map ([platform, lang]) => `${platform}:
'${lang}'`) while keeping the surrounding logic intact to match the pattern used
in utils.ts and types.ts (refer to platformLang, config.platformLangs, and
SupportedLang.CPP).
In `@src/generate-nitro-package.ts`:
- Around line 199-204: The Kotlin presence check is duplicated; add a private
helper on the class (e.g., private usesKotlin(): boolean) that returns
Object.values(this.config.platformLangs).includes(SupportedLang.KOTLIN), then
replace the duplicated expressions in updatePackageJsonConfig (the
codegen/postcodegen conditional) and installDependenciesAndRunCodegen with calls
to usesKotlin() to centralize the logic and improve maintainability.
In `@src/types.ts`:
- Around line 27-30: Remove the unused PlatformLang type definition: locate the
exported type PlatformLang in src/types.ts and delete its declaration so the
codebase only exposes PlatformLangMap (the correct type). Ensure there are no
remaining references to PlatformLang; if any tests or imports reference it,
update them to use PlatformLangMap or the appropriate type.
In `@src/utils.ts`:
- Around line 120-122: Remove the unused exported function validateTemplate from
src/utils.ts: delete the export const validateTemplate = (answer: string[]) =>
... declaration, and then run a project-wide search for "validateTemplate" to
remove any stale imports/usages (or update them if found) and adjust exports if
this was the only export in the module so module consumers still import valid
symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5112bdc2-9799-4875-9552-67506838e6e5
📒 Files selected for processing (7)
src/cli/create.tssrc/file-generators/android-file-generator.tssrc/file-generators/ios-file-generator.tssrc/file-generators/js-file-generator.tssrc/generate-nitro-package.tssrc/types.tssrc/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/generate-nitro-package.ts (1)
199-203: Codegen command construction is duplicated.The Kotlin-conditional codegen string is built in two places, which can drift over time.
Suggested refactor
+ private buildCodegenCommand() { + const packageManager = + this.config.pm === 'npm' ? 'npx --yes' : this.config.pm + const needsPostScript = Object.values(this.config.platformLangs).includes( + SupportedLang.KOTLIN + ) + return `${packageManager} nitrogen --logLevel="debug" && ${this.config.pm} run build${needsPostScript ? ' && node post-script.js' : ''}` + }Then reuse it in both script generation and install flow.
Also applies to: 522-525
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generate-nitro-package.ts` around lines 199 - 203, The Kotlin-specific fragment for the codegen command is duplicated between the codegen property and postcodegen logic; extract a single helper or local variable (e.g., kotlinPostScriptSuffix or buildKotlinPostScript()) that computes the suffix " && node post-script.js" when this.config.platformLangs includes SupportedLang.KOTLIN, then use that helper in the codegen assignment and inside getPostCodegenScript()/postcodegen logic (and the other occurrence around the install flow) so both places reuse the same computed string and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/generate-nitro-package.ts`:
- Around line 81-99: Before making generation decisions, validate that
this.config.platforms and this.config.platformLangs are consistent: check that
every platform listed in this.config.platforms has an entry in
this.config.platformLangs (and vice versa) and that values are valid
SupportedLang entries; if the invariant fails, throw (or return a clear error)
to stop execution. Add this guard right before the existing logic that computes
langs and allCpp (i.e., before using SupportedLang/SUPPORTED checks and before
calling cppGenerator.generate, cppGenerator.generateCppCodeFiles,
iosGenerator.generate, or androidGenerator.generate), and include a descriptive
error message indicating the mismatched platform keys so callers can fix the
configuration.
In `@src/utils.ts`:
- Around line 89-96: The current check uses langs.every(lang => lang ===
SupportedLang.CPP) which returns true for empty or single-entry maps and causes
an over-permissive "all" shorthand; change the condition to ensure both
platforms are explicitly configured before emitting the "all" shorthand (e.g.,
require langs.length === 2 && langs.every(lang => lang === SupportedLang.CPP) or
verify the platform keys count equals the expected number), then return the same
object using moduleName and className only when that stricter condition passes.
---
Nitpick comments:
In `@src/generate-nitro-package.ts`:
- Around line 199-203: The Kotlin-specific fragment for the codegen command is
duplicated between the codegen property and postcodegen logic; extract a single
helper or local variable (e.g., kotlinPostScriptSuffix or
buildKotlinPostScript()) that computes the suffix " && node post-script.js" when
this.config.platformLangs includes SupportedLang.KOTLIN, then use that helper in
the codegen assignment and inside getPostCodegenScript()/postcodegen logic (and
the other occurrence around the install flow) so both places reuse the same
computed string and avoid drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b443d763-f7f7-4bbc-b09c-711af6c98c69
📒 Files selected for processing (3)
assets/template/package.jsonsrc/generate-nitro-package.tssrc/utils.ts
Comment on lines
+81
to
99
| const langs = Object.values(this.config.platformLangs) | ||
| const allCpp = | ||
| langs.length > 0 && langs.every(l => l === SupportedLang.CPP) | ||
|
|
||
| if (allCpp) { | ||
| // All platforms use C++ — use composite CppFileGenerator | ||
| await this.cppGenerator.generate(this.config) | ||
| } else { | ||
| // Mixed or native-only: generate C++ files if any platform uses C++, | ||
| // then run per-platform generators individually | ||
| if (langs.includes(SupportedLang.CPP)) { | ||
| await this.cppGenerator.generateCppCodeFiles(this.config) | ||
| } | ||
| if (this.config.platformLangs[SupportedPlatform.IOS] != null) { | ||
| await this.iosGenerator.generate(this.config) | ||
| } | ||
| if (this.config.platformLangs[SupportedPlatform.ANDROID] != null) { | ||
| await this.androidGenerator.generate(this.config) | ||
| } |
There was a problem hiding this comment.
Add a config invariant check before generation.
At Line 81, generation decisions rely on platformLangs, while downstream behavior still also depends on this.config.platforms. If these drift, output can be partially generated or inconsistent.
Suggested guard
async createNitroModule() {
await createFolder(this.config.cwd)
+ for (const platform of this.config.platforms) {
+ if (this.config.platformLangs[platform] == null) {
+ throw new Error(
+ `Missing language selection for platform: ${platform}`
+ )
+ }
+ }
const langs = Object.values(this.config.platformLangs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/generate-nitro-package.ts` around lines 81 - 99, Before making generation
decisions, validate that this.config.platforms and this.config.platformLangs are
consistent: check that every platform listed in this.config.platforms has an
entry in this.config.platformLangs (and vice versa) and that values are valid
SupportedLang entries; if the invariant fails, throw (or return a clear error)
to stop execution. Add this guard right before the existing logic that computes
langs and allCpp (i.e., before using SupportedLang/SUPPORTED checks and before
calling cppGenerator.generate, cppGenerator.generateCppCodeFiles,
iosGenerator.generate, or androidGenerator.generate), and include a descriptive
error message indicating the mismatched platform keys so callers can fix the
configuration.
Comment on lines
+89
to
96
| // If all platforms use C++, use the "all" shorthand | ||
| if (langs.every(lang => lang === SupportedLang.CPP)) { | ||
| return { | ||
| [moduleName]: { | ||
| cpp: `Hybrid${moduleName}`, | ||
| all: { language: 'cpp', implementationClassName: className }, | ||
| }, | ||
| } | ||
| } |
There was a problem hiding this comment.
all autolinking shorthand is too broad for partial platform maps.
At Line 90, langs.every(...) becomes true for empty/single-entry maps, so all can be emitted even when both platforms are not explicitly configured. That can generate an over-permissive nitro.json entry.
Suggested fix
- const langs = Object.values(platformLangs)
-
- // If all platforms use C++, use the "all" shorthand
- if (langs.every(lang => lang === SupportedLang.CPP)) {
+ const iosLang = platformLangs[SupportedPlatform.IOS]
+ const androidLang = platformLangs[SupportedPlatform.ANDROID]
+
+ // Only use "all" when both supported platforms are explicitly C++
+ if (iosLang === SupportedLang.CPP && androidLang === SupportedLang.CPP) {
return {
[moduleName]: {
all: { language: 'cpp', implementationClassName: className },
},
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils.ts` around lines 89 - 96, The current check uses langs.every(lang
=> lang === SupportedLang.CPP) which returns true for empty or single-entry maps
and causes an over-permissive "all" shorthand; change the condition to ensure
both platforms are explicitly configured before emitting the "all" shorthand
(e.g., require langs.length === 2 && langs.every(lang => lang ===
SupportedLang.CPP) or verify the platform keys count equals the expected
number), then return the same object using moduleName and className only when
that stricter condition passes.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/generate-nitro-package.ts (1)
81-99:⚠️ Potential issue | 🟠 MajorAdd config invariant validation before branching generation logic.
Generation decisions here mix
platformLangs(this file) andplatforms(insidesrc/file-generators/cpp-file-generator.ts:20-68). Without a guard, mismatches can produce partial or inconsistent output.Suggested guard
async createNitroModule() { await createFolder(this.config.cwd) + + const platformSet = new Set(this.config.platforms) + const langPlatforms = Object.keys(this.config.platformLangs) as SupportedPlatform[] + + for (const p of this.config.platforms) { + if (this.config.platformLangs[p] == null) { + throw new Error(`Missing language selection for platform: ${p}`) + } + } + for (const p of langPlatforms) { + if (!platformSet.has(p)) { + throw new Error(`Language provided for unselected platform: ${p}`) + } + } const langs = Object.values(this.config.platformLangs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/generate-nitro-package.ts` around lines 81 - 99, Before running the branching generation logic, validate the config invariant that this.config.platformLangs is consistent with this.config.platforms (the same set of platforms and no missing/null entries) to avoid partial output; add a guard near the start of the method that checks keys/values of platformLangs against platforms (or vice versa) and either normalize them or throw a clear Error if inconsistent, then only proceed to call cppGenerator.generate, cppGenerator.generateCppCodeFiles, iosGenerator.generate, and androidGenerator.generate when the validation passes.src/utils.ts (1)
89-90:⚠️ Potential issue | 🟠 Major
allshorthand condition is still over-broad.At Line 90,
every(...)still returns true for partial platform maps, so{ all: ... }can be emitted when only one platform is configured. Require explicit CPP on both iOS and Android before usingall.Suggested fix
- const langs = Object.values(platformLangs) - - // If all platforms use C++, use the "all" shorthand - if (langs.every(lang => lang === SupportedLang.CPP)) { + const iosLang = platformLangs[SupportedPlatform.IOS] + const androidLang = platformLangs[SupportedPlatform.ANDROID] + + // Use "all" only when both platforms are explicitly C++ + if (iosLang === SupportedLang.CPP && androidLang === SupportedLang.CPP) { return { [moduleName]: { all: { language: SupportedLang.CPP, implementationClassName: className, }, }, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils.ts` around lines 89 - 90, The current condition using langs.every(lang => lang === SupportedLang.CPP) is too broad and can emit the "{ all: ... }" shorthand for partial platform maps; change it to require explicit C++ on both iOS and Android before emitting "all" by verifying the platforms list contains both "ios" and "android" and that both corresponding entries are SupportedLang.CPP (e.g., replace the langs.every(...) check with a check that platforms includes ios and android and that langs for those platforms === SupportedLang.CPP); update the emission site where "{ all: ... }" is produced to rely on this stricter condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/generate-nitro-package.ts`:
- Around line 91-93: The mixed C++ flow can fail because generateCppCodeFiles
(invoked via this.cppGenerator.generateCppCodeFiles in the SupportedLang.CPP
branch) writes files under a cpp/ directory that may not exist; update the flow
to ensure the target cpp directory is created before generateCppCodeFiles runs
(e.g., create the cpp directory path with recursive mkdir or equivalent and
handle/propagate errors), or modify generateCppCodeFiles itself to create its
output directory if missing, so writes cannot throw ENOENT.
---
Duplicate comments:
In `@src/generate-nitro-package.ts`:
- Around line 81-99: Before running the branching generation logic, validate the
config invariant that this.config.platformLangs is consistent with
this.config.platforms (the same set of platforms and no missing/null entries) to
avoid partial output; add a guard near the start of the method that checks
keys/values of platformLangs against platforms (or vice versa) and either
normalize them or throw a clear Error if inconsistent, then only proceed to call
cppGenerator.generate, cppGenerator.generateCppCodeFiles, iosGenerator.generate,
and androidGenerator.generate when the validation passes.
In `@src/utils.ts`:
- Around line 89-90: The current condition using langs.every(lang => lang ===
SupportedLang.CPP) is too broad and can emit the "{ all: ... }" shorthand for
partial platform maps; change it to require explicit C++ on both iOS and Android
before emitting "all" by verifying the platforms list contains both "ios" and
"android" and that both corresponding entries are SupportedLang.CPP (e.g.,
replace the langs.every(...) check with a check that platforms includes ios and
android and that langs for those platforms === SupportedLang.CPP); update the
emission site where "{ all: ... }" is produced to rely on this stricter
condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 443908e1-6592-4c37-bb17-b6436df81fc7
📒 Files selected for processing (5)
src/constants.tssrc/file-generators/android-file-generator.tssrc/file-generators/cpp-file-generator.tssrc/generate-nitro-package.tssrc/utils.ts
Comment on lines
+91
to
+93
| if (langs.includes(SupportedLang.CPP)) { | ||
| await this.cppGenerator.generateCppCodeFiles(this.config) | ||
| } |
There was a problem hiding this comment.
Mixed C++ flow can fail due to missing cpp/ directory.
At Line 92, generateCppCodeFiles() writes cpp/*.cpp and cpp/*.hpp but does not create cpp/. In mixed-language scenarios this can throw ENOENT.
Suggested fix
if (langs.includes(SupportedLang.CPP)) {
+ await createFolder(this.config.cwd, 'cpp')
await this.cppGenerator.generateCppCodeFiles(this.config)
}📝 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.
Suggested change
| if (langs.includes(SupportedLang.CPP)) { | |
| await this.cppGenerator.generateCppCodeFiles(this.config) | |
| } | |
| if (langs.includes(SupportedLang.CPP)) { | |
| await createFolder(this.config.cwd, 'cpp') | |
| await this.cppGenerator.generateCppCodeFiles(this.config) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/generate-nitro-package.ts` around lines 91 - 93, The mixed C++ flow can
fail because generateCppCodeFiles (invoked via
this.cppGenerator.generateCppCodeFiles in the SupportedLang.CPP branch) writes
files under a cpp/ directory that may not exist; update the flow to ensure the
target cpp directory is created before generateCppCodeFiles runs (e.g., create
the cpp directory path with recursive mkdir or equivalent and handle/propagate
errors), or modify generateCppCodeFiles itself to create its output directory if
missing, so writes cannot throw ENOENT.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cli/create.ts (1)
127-134: Consider simplifying the View type default language resolution.For
Nitro.View,resolveViewLanguagesalready returns aPlatformLangMap. Converting it to an array viaObject.values()and then back to a map viagetPlatformLangMapis redundant.♻️ Proposed simplification
if (!value) { + if (packageType === Nitro.View) { + return resolveViewLanguages(platforms) + } return getPlatformLangMap( platforms, - packageType === Nitro.View - ? Object.values(resolveViewLanguages(platforms)) - : allowedSelections[0] + allowedSelections[0] ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/create.ts` around lines 127 - 134, The current default-language branch for Nitro.View unnecessarily converts resolveViewLanguages(platforms) to an array and back to a map; replace that branch to return resolveViewLanguages(platforms) directly. Update the conditional in the function that uses Nitro.View so that when packageType === Nitro.View it returns resolveViewLanguages(platforms), otherwise keep returning getPlatformLangMap(platforms, allowedSelections[0]); ensure any expected type is satisfied by resolveViewLanguages so no extra conversion is needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/cli/create.ts`:
- Around line 127-134: The current default-language branch for Nitro.View
unnecessarily converts resolveViewLanguages(platforms) to an array and back to a
map; replace that branch to return resolveViewLanguages(platforms) directly.
Update the conditional in the function that uses Nitro.View so that when
packageType === Nitro.View it returns resolveViewLanguages(platforms), otherwise
keep returning getPlatformLangMap(platforms, allowedSelections[0]); ensure any
expected type is satisfied by resolveViewLanguages so no extra conversion is
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96626207-c0d2-4e61-8dca-50c3c39af94d
📒 Files selected for processing (3)
src/cli/create.tssrc/code-snippets/code.js.tssrc/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/types.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli/create.ts (1)
138-143: Simplify the default View branch to return the map directly.This currently converts map → values → map, which is redundant.
Refactor suggestion
if (!value) { - return getPlatformLangMap( - platforms, - packageType === Nitro.View - ? Object.values(resolveViewLanguages(platforms)) - : allowedSelections[0] - ) + if (packageType === Nitro.View) { + return resolveViewLanguages(platforms) + } + return getPlatformLangMap(platforms, allowedSelections[0]) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/create.ts` around lines 138 - 143, The current ternary in the call to getPlatformLangMap unnecessarily converts a map to values then back to a map for the Nitro.View branch; change the ternary so when packageType === Nitro.View it passes the map returned by resolveViewLanguages(platforms) directly (instead of Object.values(resolveViewLanguages(platforms))) so getPlatformLangMap receives the correct structure; update the expression around getPlatformLangMap(...) accordingly, referencing getPlatformLangMap, resolveViewLanguages, packageType and Nitro.View.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli/create.ts`:
- Around line 220-223: The code builds modulePath using process.cwd() which
ignores the configured output directory (--module-dir) and can check/delete the
wrong folder; update the path resolution so modulePath is created from the
resolved module directory option (the configured output directory provided by
the CLI, i.e. the --module-dir value) instead of process.cwd(), and use that
same resolved directory when doing existence checks and cleanup (the places
referencing modulePath/packageName and the cleanup logic around lines 273-274)
so all checks and deletions operate on the user-specified output folder.
---
Nitpick comments:
In `@src/cli/create.ts`:
- Around line 138-143: The current ternary in the call to getPlatformLangMap
unnecessarily converts a map to values then back to a map for the Nitro.View
branch; change the ternary so when packageType === Nitro.View it passes the map
returned by resolveViewLanguages(platforms) directly (instead of
Object.values(resolveViewLanguages(platforms))) so getPlatformLangMap receives
the correct structure; update the expression around getPlatformLangMap(...)
accordingly, referencing getPlatformLangMap, resolveViewLanguages, packageType
and Nitro.View.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 443dd2f4-d348-42a4-b455-9fb09dbb92b1
📒 Files selected for processing (3)
.github/workflows/ci-packages.ymlsrc/cli/create.tssrc/file-generators/cpp-file-generator.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/file-generators/cpp-file-generator.ts
Comment on lines
220
to
223
| const modulePath = path.join( | ||
| process.cwd(), | ||
| 'react-native-' + packageName.toLowerCase() | ||
| `react-native-${packageName.toLowerCase()}` | ||
| ) |
There was a problem hiding this comment.
Use the configured output directory for existence checks and cleanup.
Line 220 and Line 273 hardcode process.cwd(). With --module-dir, this can check/delete the wrong folder (including unrelated directories) while generation runs elsewhere.
Proposed fix
export const createModule = async (
packageName: string,
options: CreateModuleOptions
) => {
let packageType = Nitro.Module
let moduleFactory: NitroModuleFactory | null = null
let spinnerStarted = false
+ let shouldCleanup = false
const spinner = p.spinner()
+ const outputRoot = options.moduleDir || process.cwd()
try {
@@
moduleFactory = new NitroModuleFactory({
@@
- cwd: options.moduleDir || process.cwd(),
+ cwd: outputRoot,
@@
const modulePath = path.join(
- process.cwd(),
+ outputRoot,
`react-native-${packageName.toLowerCase()}`
)
@@
spinner.start(
messages.creating.replace('{packageType}', capitalize(packageType))
)
spinnerStarted = true
+ shouldCleanup = true
await moduleFactory.createNitroModule()
@@
} catch (error) {
- if (packageName) {
+ if (shouldCleanup && packageName) {
const modulePath = path.join(
- process.cwd(),
+ outputRoot,
`react-native-${packageName.toLowerCase()}`
)
rmSync(modulePath, { recursive: true, force: true })
}Also applies to: 273-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli/create.ts` around lines 220 - 223, The code builds modulePath using
process.cwd() which ignores the configured output directory (--module-dir) and
can check/delete the wrong folder; update the path resolution so modulePath is
created from the resolved module directory option (the configured output
directory provided by the CLI, i.e. the --module-dir value) instead of
process.cwd(), and use that same resolved directory when doing existence checks
and cleanup (the places referencing modulePath/packageName and the cleanup logic
around lines 273-274) so all checks and deletions operate on the user-specified
output folder.
Owner
|
Thank you @DaveyEke! |
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
2 participants
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 PR updates
create-nitro-moduleto work with the latest syntax changes Marc introduced tonitro.jsonin Nitro modulesSummary by CodeRabbit