refactor(api): vendor node_modules instead of pnpm store
#1346
Conversation
WalkthroughThe changes update the deployment preparation process. In api/scripts/build.ts, the assignment of the Changes
Possibly related PRs
Suggested reviewers
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 (4)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 0
🧹 Nitpick comments (2)
api/scripts/build.ts (1)
50-51: Retain pnpm Configuration:
With the removal of any deletion logic for thepnpmproperty, the script now preserves this configuration—thereby helping ensure that postinstall scripts (and other pnpm settings) run in production as intended. Consider updating the comment at line 50 to clearly reflect that thepackage.jsonis being written back with all pnpm properties intact.package.json (1)
23-32: Updated pnpm.onlyBuiltDependencies List:
The dependency list has been adjusted by removing@nestjs/core,cpu-features, andnestjs-pinoand by addingprotobufjs. Ensure that these changes align with your deployment requirements and that no essential dependency for building or postinstall operations is inadvertently removed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
api/scripts/build.ts(1 hunks)package.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Test API
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (3)
133-146: Added node_modules archive restoration logicGood performance optimization! The function now checks for a cached node_modules archive and attempts to restore from it before running pnpm install.
Consider adding a timestamp check to verify the archive isn't outdated:
if [ -f "$archive_file" ]; then + # Check if archive is recent (within 7 days) + if [ $(( $(date +%s) - $(stat -c %Y "$archive_file") )) -gt 604800 ]; then + echo "Archive is older than 7 days. Running fresh install..." + else echo "Found node_modules archive at '$archive_file'. Attempting to restore..." rm -rf "$node_modules_dir" if tar -xzf "$archive_file" -C "$(dirname "$node_modules_dir")" --preserve-permissions; then echo "Successfully restored node_modules from archive." return 0 else echo "Failed to restore from archive. Falling back to pnpm install..." fi + fi fi
160-183: New archive_node_modules functionThe function correctly archives the node_modules directory with appropriate error handling. Using compression level 1 is a good balance between speed and size.
Consider adding disk space verification similar to what you have in the restore_pnpm_store function:
# Create destination directory if it doesn't exist mkdir -p "$dest_dir" +# Check available disk space in destination +local source_size +source_size=$(du -sb "$source_dir" | cut -f1) +local dest_space +dest_space=$(df --output=avail "$(dirname "$archive_file")" | tail -n1) +dest_space=$((dest_space * 1024)) # Convert KB to bytes + +if [ "$dest_space" -lt "$source_size" ]; then + echo "Error: Insufficient disk space for archiving. Need at least $((source_size / 1024 / 1024))MB, have $((dest_space / 1024 / 1024))MB" + return 1 +fi + echo "Archiving node_modules from '$source_dir' to '$archive_file'"
152-153: Added warning and archive call after installationThe warning helps prevent users from interrupting the archiving process, but lacks clarity about what's happening.
Make the warning more informative:
-echo "⚠️ Do not close this window yet" +echo "⚠️ Do not close this window yet - creating node_modules archive for future use" archive_node_modules
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
plugin/plugins/dynamix.unraid.net.plg(1 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/plugins/dynamix.unraid.net.plg
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)
204-206: Added archive-dependencies command handlerThis addition properly exposes the new archiving functionality through the command interface.
111-111:❓ Verification inconclusive
Changed from --prefer-offline to --offline mode
The installation command now runs in strict offline mode instead of prefer-offline. This ensures faster installation from cache but will fail if packages aren't available locally.
🏁 Script executed:
#!/bin/bash # Check if there are specific reasons mentioned for the strict offline mode change grep -r "offline.*mode" --include="*.md" --include="*.txt" .Length of output: 62
Note on the Offline Mode Change:
The installation command was updated to use strict offline mode (--offline) instead of the--prefer-offlineflag. This change is intended to speed up builds by enforcing the use of a local cache, though it will fail if any required packages aren’t available locally. Our search in markdown and text documentation did not yield any further details explaining this change. Please verify manually that the local cache includes all necessary packages and that this behavior aligns with your deployment strategy.• Confirm that no additional context exists elsewhere in the repository (e.g., commit messages or internal documentation) regarding this change.
• Ensure that the local cache is complete so that package installations won’t unexpectedly fail.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugin/builder/build-pnpm-store.ts (2)
20-42: Update function name and documentation to match new implementationWhile the implementation changes are correct, the function name and documentation still reference PNPM store, which is no longer accurate:
- The function name
bundlePnpmStoredoesn't reflect its current purpose- The JSDoc comments still mention PNPM store and packed-pnpm-store.txz
Consider updating these to maintain clarity and consistency.
/** - * The name of the pnpm store archive that will be vendored with the plugin. + * The name of the node modules archive that will be vendored with the plugin. * @returns The name of the pnpm store bundle file *//** - * Prepare a versioned bundle of the API's pnpm store to vendor dependencies. + * Prepare a versioned bundle of the API's node modules to vendor dependencies. * - * It expects a generic `packed-pnpm-store.txz` archive to be available in the `startingDir`. + * It expects a generic `packed-node-modules.tar.xz` archive to be available in the `startingDir`. * It copies this archive to the `deployDir` directory and adds a version to the filename. - * It does not actually create the packed pnpm store archive; that is done inside the API's build script. + * It does not actually create the packed node modules archive; that is done inside the API's build script. * * After this operation, the vendored store will be available inside the `deployDir`. */ -export async function bundlePnpmStore(): Promise<void> { +export async function bundleNodeModules(): Promise<void> {
1-42: Consider renaming this fileThe filename
build-pnpm-store.tsno longer accurately represents the file's purpose since it now deals with node modules rather than PNPM store. Consider renaming it to something likebuild-node-modules.tsfor clarity and consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
.github/workflows/build-plugin.yml(2 hunks).github/workflows/main.yml(1 hunks)api/scripts/build.ts(2 hunks)plugin/builder/build-pnpm-store.ts(2 hunks)plugin/docker-compose.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/scripts/build.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugin/builder/build-pnpm-store.ts (1)
plugin/builder/utils/consts.ts (1)
startingDir(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build Web App
- GitHub Check: Cloudflare Pages
🔇 Additional comments (6)
plugin/docker-compose.yml (1)
15-15: File path updated to reflect new node modules approachThe volume mapping has been updated to use packed-node-modules.tar.xz instead of packed-pnpm-store.txz, which is consistent with the shift from PNPM store to node_modules for dependency management.
.github/workflows/build-plugin.yml (2)
106-107: Artifact name updated to reflect new node modules approachThe artifact name has been updated from packed-pnpm-store to packed-node-modules, aligning with the shift from PNPM store to node_modules.
133-136: Added verification for tar.xz fileGood addition of error checking for the .tar.xz file in the deploy directory. This ensures the node_modules archive exists before proceeding with the build process.
.github/workflows/main.yml (1)
194-199: Updated step name and artifact detailsThe GitHub action step has been correctly updated to use the new node_modules approach:
- Step name changed from "Upload PNPM Store" to "Upload Node Modules"
- Artifact name updated to packed-node-modules
- Path updated to use packed-node-modules.tar.xz
These changes are consistent with the overall shift in dependency management approach.
plugin/builder/build-pnpm-store.ts (2)
26-26: Bundle name updated to use node modulesThe bundle name format has been updated to reflect the shift from pnpm store to node modules.
39-39: Store archive path updatedThe store archive path has been updated to use packed-node-modules.tar.xz instead of packed-pnpm-store.txz.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
api/package.json (2)
205-207: Review: Verify the override configuration for @as-integrations/fastify
The new override maps thefastifydependency to the variable$fastify. Please confirm that this variable-based aliasing is intentional and correctly supported by your dependency resolution process. If this mapping deviates from the typical version string override, adding a brief comment or documentation note could help future maintainers.
208-211: Review: Confirm alias resolution for nest-authz dependencies
The override fornest-authzdirects@nestjs/commonand@nestjs/coreto resolve to$@nestjs/commonand$@nestjs/corerespectively. Verify that these substitutions work as expected in all environments (including production) and consider adding documentation on this non-standard aliasing approach for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (2)
api/package.json(1 hunks)api/scripts/build.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- api/scripts/build.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (1)
95-100:⚠️ Potential issueUpdate extraction command to match archive format
Since the archive format has changed from tar.xz to tar.gz as indicated in the documentation, the extraction command should use
-zinstead of-J.- if ! tar -xJf "$backup_file" -C "$(dirname "$pnpm_store_dir")" --preserve-permissions; then + if ! tar -xzf "$backup_file" -C "$(dirname "$pnpm_store_dir")" --preserve-permissions; then
🧹 Nitpick comments (3)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (3)
11-11: Consider renaming the variable to match its new purposeThe variable name
pnpm_store_dirnow points to a node_modules directory, which is misleading. Consider renaming it tonode_modules_dirto better reflect its actual purpose.-pnpm_store_dir="/usr/local/unraid-api/node_modules" +node_modules_dir="/usr/local/unraid-api/node_modules"
152-152: Make warning message more informativeThe warning message "Do not close this window yet" doesn't explain why users should keep the window open.
- echo "⚠️ Do not close this window yet" + echo "⚠️ Do not close this window yet - archiving node_modules in progress"
160-183: Well-structured archive function with good error handlingThe new
archive_node_modulesfunction is well-structured with proper error handling and uses fast compression (level 1), which is appropriate for potentially large node_modules directories.Consider parameterizing the source and destination paths for better flexibility:
archive_node_modules() { - local source_dir="/usr/local/unraid-api/node_modules" - local dest_dir="/boot/config/plugins/dynamix.my.servers" + local source_dir="${node_modules_dir:-/usr/local/unraid-api/node_modules}" + local dest_dir="${flash:-/boot/config/plugins/dynamix.my.servers}" local archive_file="${dest_dir}/node_modules.tar.gz"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
api/scripts/build.ts(2 hunks)plugin/plugins/dynamix.unraid.net.plg(3 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/scripts/build.ts
- plugin/plugins/dynamix.unraid.net.plg
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Test API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (2)
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (2)
111-111: Be aware of stricter offline installationChanging from
--prefer-offlineto--offlinemakes the installation strictly offline with no fallback to online mode. This is more reliable in controlled environments but will fail if packages aren't available locally.
204-206: Good addition of archive commandThe new
archive-dependenciescommand provides a logical way to manually trigger the archiving process, maintaining consistency with the existing command structure.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugin/builder/build-vendor-store.ts (1)
13-18: Consider enhancing error handling.The function correctly retrieves the version, but could benefit from more specific error handling.
function getVersion(): string { const packageJsonPath = join(apiDir, "package.json"); - const packageJsonString = readFileSync(packageJsonPath, "utf8"); - const packageJson = JSON.parse(packageJsonString); - return packageJson.version; + try { + const packageJsonString = readFileSync(packageJsonPath, "utf8"); + const packageJson = JSON.parse(packageJsonString); + if (!packageJson.version) { + throw new Error("Version not found in package.json"); + } + return packageJson.version; + } catch (error) { + throw new Error(`Failed to read API version: ${error instanceof Error ? error.message : String(error)}`); + } }plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (1)
104-109: Consider validating the restore-dependencies argument.While the command case structure has been updated correctly, it would be beneficial to validate the backup file path before passing it to the function.
'restore-dependencies') - restore_dependencies "$2" + if [ -z "$2" ]; then + echo "Error: Missing backup file path. Usage: $0 restore-dependencies <backup_file_path>" + exit 1 + fi + restore_dependencies "$2" ;; 'archive-dependencies') archive_dependencies
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
api/scripts/build.ts(1 hunks)plugin/builder/build-plugin.ts(3 hunks)plugin/builder/build-vendor-store.ts(1 hunks)plugin/plugins/dynamix.unraid.net.plg(3 hunks)plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- api/scripts/build.ts
- plugin/plugins/dynamix.unraid.net.plg
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugin/builder/build-plugin.ts (2)
plugin/builder/utils/bucket-urls.ts (1)
getAssetUrl(33-37)plugin/builder/build-vendor-store.ts (2)
getVendorBundleName(24-27)bundleVendorStore(38-42)
plugin/builder/build-vendor-store.ts (2)
plugin/builder/utils/paths.ts (2)
apiDir(19-26)deployDir(17-17)plugin/builder/utils/consts.ts (1)
startingDir(6-6)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test API
- GitHub Check: Build Web App
- GitHub Check: Build API
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
plugin/builder/build-plugin.ts (3)
15-15: Updated imports to reflect new dependency management approach.The imports have been changed to work with the new vendor store mechanism instead of the previous PNPM store approach.
64-65: Updated entity values for vendor store references.The entities have been correctly updated to use the new
getVendorBundleName()function for both URL and filename properties, maintaining consistency with the broader dependency management changes.
111-111: Updated vendor store bundling call.The function call has been updated to use
bundleVendorStore()which aligns with the project's transition from PNPM to a vendor store approach.plugin/builder/build-vendor-store.ts (1)
24-27: Well-defined function for generating vendor bundle names.This function builds a consistent and versioned filename for the node_modules archive, which will help with dependency versioning and tracking.
plugin/source/dynamix.unraid.net/etc/rc.d/rc.unraid-api (3)
11-11: Updated path to use node_modules directory.The variable has been correctly renamed and updated to point to the node_modules directory, which aligns with the project's transition from PNPM to standard npm for dependency management.
21-27: Updated function documentation for node_modules restoration.The documentation has been properly updated to reflect the new dependency approach with node_modules instead of PNPM store.
69-92: New function to archive node_modules dependencies.This well-structured function creates a compressed archive of the node_modules directory with appropriate error handling and progress messages.
|
@coderabbitai summary |
✅ Actions performedSummary regeneration triggered. |
| /etc/rc.d/rc.unraid-api restore-dependencies "$VENDOR_ARCHIVE" | ||
| echo | ||
| echo "⚠️ Do not close this window yet" | ||
| /etc/rc.d/rc.unraid-api pnpm-install |
pujitm
changed the title
node_modules instead of pnpm store
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
due to issues and redundancies in vendoring postinstall side-effects, such as compiled bindings for libvirt, we reverted to vendoring `node_modules`, installed via `npm` instead of a global pnpm store generated by `pnpm`. This should resolve runtime issues with e.g. the libvirt bindings because `node_modules` will contain the correct "side-effects." ## Summary by CodeRabbit - **New Features** - Introduced a command to remove stale archive files during the cleanup process. - Added functionality to archive the `node_modules` directory. - Enhanced dependency resolution with new overrides for specific packages. - **Chores** - Updated dependency settings by replacing one key dependency with an alternative and removing two unused ones, ensuring optimal deployment. - Enhanced the installation process to operate strictly in offline mode. - Updated artifact naming conventions for clarity and consistency in workflows. - Modified volume mappings in the Docker Compose configuration to reflect new artifact names. - Improved error handling in the GitHub Actions workflow by adding checks for required files. - Updated references in the build process to use a vendor store instead of the PNPM store. - Removed the management of PNPM store archives from the build process.
Summary by CodeRabbit
New Features
node_modulesdirectory.Chores