Feat/pnpm support #228
Feat/pnpm support #228
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughSupport for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant NitroModuleFactory
participant Utils
User->>CLI: Run CLI to create Nitro module
CLI->>Utils: detectPackageManager()
Utils-->>CLI: Returns 'pnpm' (if detected)
CLI->>User: Prompt for package manager (includes pnpm)
User-->>CLI: Selects 'pnpm'
CLI->>NitroModuleFactory: Generate package with 'pnpm'
NitroModuleFactory->>NitroModuleFactory: Configure pnpm workspace, commands, scripts
NitroModuleFactory-->>CLI: Package generated with pnpm support
Assessment against linked issues
Poem
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
commitlint.config.cjsOops! Something went wrong! :( ESLint: 9.29.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@eslint/js' imported from /eslint.config.js 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ 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 (
|
![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
🔭 Outside diff range comments (2)
src/utils.ts (1)
206-214: Annotate return type for tighterPackageManagerinference
detectPackageManagercurrently relies on TypeScript’s widening tostring, which later causesPackageManagerto degrade tostring(seesrc/types.ts).
Provide an explicit literal-union return type so downstream code keeps strict exhaustiveness checks.-export const detectPackageManager = () => { +export const detectPackageManager = (): 'npm' | 'yarn' | 'bun' | 'pnpm' | undefined => {.github/workflows/test-nitro-cli.yml (1)
293-319:pnpmmissing from Android matrix
test-android-builddefinespm: ['bun', 'yarn']but still contains conditional steps forpnpm.
Either add'pnpm'to the matrix or drop the dead steps to keep the workflow concise.- pm: ['bun', 'yarn'] + pm: ['bun', 'yarn', 'pnpm']
🧹 Nitpick comments (4)
src/generate-nitro-package.ts (4)
145-148: Considerrunfor the pnpm postcodegen script
pnpm --filter ./example podworks only whenpodis an npm-script or a binary on PATH.
If it’s an npm-script (same as npm/yarn cases) it’s safer to prependrun:- } else if (this.config.pm === 'pnpm') { - script = `pnpm --filter ./example pod` + } else if (this.config.pm === 'pnpm') { + script = `pnpm --filter ./example run pod`
192-203: Avoiddelete+ create workspace file in one shot
delete newWorkspacePackageJsonFile.workspacestriggers the Biome warning.
Cheaper:newWorkspacePackageJsonFile.workspaces = undefined.- Building and writing
pnpm-workspace.yamllooks good.- delete newWorkspacePackageJsonFile.workspaces + newWorkspacePackageJsonFile.workspaces = undefined
274-279: Usepnpm dlxinstead ofpnpx
pnpmships an alias, but the documented way ispnpm dlx. Keeps parity withbunx/npx.- : this.config.pm === 'pnpm' - ? 'pnpx' + : this.config.pm === 'pnpm' + ? 'pnpm dlx'
408-465: Duplicate write & commented-out dead code
androidSettingsGradlePathis written once (408–412) and again insidefilesToWrite.
Also, the large block of commented-outwriteFilecalls above clutters the method.- await writeFile( - androidSettingsGradlePath, - androidSettingsGradleCode(toPascalCase(this.config.packageName)), - { encoding: 'utf8' } - ) + // initial write removed – now handled in the batched `filesToWrite`Then delete the commented legacy lines (367-399, 377-386, 395-399) to keep the file clean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/test-nitro-cli.yml(6 hunks)src/cli/create.ts(2 hunks)src/generate-nitro-package.ts(8 hunks)src/types.ts(2 hunks)src/utils.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/types.ts (1)
src/utils.ts (1)
detectPackageManager(206-214)
src/generate-nitro-package.ts (3)
src/utils.ts (2)
replacePlaceholder(168-192)toPascalCase(59-64)src/code-snippets/code.js.ts (3)
metroConfig(105-119)babelConfig(121-140)exampleTsConfig(142-151)src/code-snippets/code.android.ts (1)
androidSettingsGradleCode(24-32)
🪛 Biome (1.9.4)
src/generate-nitro-package.ts
[error] 202-202: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: Test iOS Build - pnpm - view (Debug)
- GitHub Check: Test iOS Build - pnpm - module (Debug)
- GitHub Check: Test iOS Build - pnpm - module (Release)
- GitHub Check: Test iOS Build - yarn - view (Release)
- GitHub Check: Test Android Build - yarn - view (Debug)
- GitHub Check: Test Android Build - yarn - view (Release)
- GitHub Check: Test iOS Build - yarn - module (Release)
- GitHub Check: Test Android Build - bun - module (Debug)
- GitHub Check: Test Android Build - yarn - module (Release)
- GitHub Check: Test Android Build - bun - view (Debug)
- GitHub Check: Test Android Build - bun - module (Release)
- GitHub Check: Test Android Build - yarn - module (Debug)
- GitHub Check: Test Android Build - bun - view (Release)
🔇 Additional comments (2)
src/cli/create.ts (2)
147-148: Default topnpmin CI looks goodSwitching the CI fallback from
bun➜pnpmaligns with the new matrix – no concerns.
253-256:pnpmoption added to the prompt – LGTMInteractive users can now pick
pnpm; implementation mirrors the other options.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/generate-nitro-package.ts (2)
202-207: Nitpick: double-space in constant & delete operator
- The double space before
NPMRC_FILENAMEwill trip most linters.- Static-analysis flagged
delete– fine here, but an assignment to
undefinedis slightly faster and avoids the warning.- const NPMRC_FILENAME = '.npmrc' + const NPMRC_FILENAME = '.npmrc' … - delete newWorkspacePackageJsonFile.workspaces + newWorkspacePackageJsonFile.workspaces = undefined
414-421: Duplicate writes – Android gradle file is written twice
androidSettingsGradlePathis written at line 414 and again inside
filesToWrite. Remove the first write or exclude it from the batch to
avoid redundant I/O.Also applies to: 450-465
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/generate-nitro-package.ts(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/generate-nitro-package.ts (4)
assets/react-native.config.js (1)
path(1-1)src/utils.ts (2)
replacePlaceholder(168-192)toPascalCase(59-64)src/code-snippets/code.js.ts (3)
metroConfig(105-119)babelConfig(121-140)exampleTsConfig(142-151)src/code-snippets/code.android.ts (1)
androidSettingsGradleCode(24-32)
🪛 Biome (1.9.4)
src/generate-nitro-package.ts
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (1)
src/generate-nitro-package.ts (1)
228-231: Install-command placeholder looks goodThe switch now yields
pnpm add,bun add, etc. Nice touch.
| const packageManager = | ||
| this.config.pm === 'bun' | ||
| ? 'bunx' | ||
| : this.config.pm === 'pnpm' | ||
| ? 'pnpx' | ||
| : 'npx -y' | ||
|
|
There was a problem hiding this comment.
Use pnpm dlx, not pnpx, when the selected PM is pnpm
pnpx belongs to npm; on a pnpm-only environment it won’t exist.
pnpm dlx (or pnpm exec) is the correct analogue.
- : this.config.pm === 'pnpm'
- ? 'pnpx'
+ : this.config.pm === 'pnpm'
+ ? 'pnpm dlx'📝 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.
| const packageManager = | |
| this.config.pm === 'bun' | |
| ? 'bunx' | |
| : this.config.pm === 'pnpm' | |
| ? 'pnpx' | |
| : 'npx -y' | |
| const packageManager = | |
| this.config.pm === 'bun' | |
| ? 'bunx' | |
| : this.config.pm === 'pnpm' | |
| ? 'pnpm dlx' | |
| : 'npx -y' |
🤖 Prompt for AI Agents
In src/generate-nitro-package.ts around lines 279 to 285, replace the use of
'pnpx' with 'pnpm dlx' when the package manager is pnpm, because 'pnpx' is an
npm tool and may not be available in pnpm-only environments. Update the
conditional expression to use 'pnpm dlx' instead of 'pnpx' to correctly invoke
pnpm's execution command.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/generate-nitro-package.ts (1)
414-471: Duplicate writes — same files are written twice
androidSettingsGradlePath(and others) are individually written at 414-418 and then again viafilesToWritein thePromise.allblock (450-463).
This is redundant I/O and invites inconsistent states if one write fails.Option 1 (cleaner): remove the earlier individual
writeFilecalls and keep the batchedPromise.all.- await writeFile( - androidSettingsGradlePath, - androidSettingsGradleCode(toPascalCase(this.config.packageName)), - { encoding: 'utf8' } - )Option 2: keep the single writes and drop them from the list in
filesToWrite.Pick one path to avoid duplication.
♻️ Duplicate comments (2)
src/generate-nitro-package.ts (2)
279-285: Usepnpm dlx, notpnpx, for one-off CLI execution
This was already raised in the earlier review but is still unresolved.
pnpxis shipped with npm, not pnpm; it will be missing in a pnpm-only setup.
Replace withpnpm dlx(orpnpm exec) to ensure compatibility.- : this.config.pm === 'pnpm' - ? 'pnpx' + : this.config.pm === 'pnpm' + ? 'pnpm dlx'
145-147:pnpmcommand is still missing the requiredrunkeyword
pnpmexecutes workspace-local scripts viapnpm run ….
Withoutrun,pnpmwill attempt to treatpodas a package to install, resulting in failure.- } else if (this.config.pm === 'pnpm') { - script = `pnpm --filter ./example pod` + } else if (this.config.pm === 'pnpm') { + script = `pnpm --filter ./example run pod`
🧹 Nitpick comments (1)
src/generate-nitro-package.ts (1)
192-209: Prefer property assignment overdeleteto avoid V8 de-optStatic-analysis flagged
delete newWorkspacePackageJsonFile.workspaces.
Assigningundefinedkeeps the object shape stable and is marginally faster.- delete newWorkspacePackageJsonFile.workspaces + newWorkspacePackageJsonFile.workspaces = undefined
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/generate-nitro-package.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/generate-nitro-package.ts
[error] 208-208: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
BREAKING CHANGE: The CLI entry point 'nitro-module' is no longer available in the package's bin field.
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #221
Summary by CodeRabbit
New Features
Improvements
Chores