chore(api): enable using workspace plugins in production #1343
Conversation
WalkthroughThis pull request updates multiple components of the API project. It modifies the post-build process by extending the script in Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Postbuild Script
participant CP as copy-plugins.js
participant FS as File System
participant Plugin as Workspace Plugin
Build->>CP: Run postbuild (includes executing copy-plugins.js)
CP->>FS: Read package.json & ensure dist/plugins exists
CP->>CP: Filter workspace plugins by prefix
loop For each plugin
CP->>Plugin: Execute 'pnpm build'
Plugin-->>CP: Build result
alt Build successful
CP->>FS: Copy plugin dist folder to dist/plugins directory
else Build failed
CP->>CP: Log error and exit process
end
end
CP->>FS: Log success of all copy operations
sequenceDiagram
participant Service as PluginService
participant Src1 as Primary Package
participant Src2 as Local Index File
participant Logger as Debug Logger
Service->>Service: Identify plugin from peerDependencies
Service->>Src1: Attempt import using package name
Service->>Src2: Attempt import using local index path
par
Src1-->>Service: Successful import (if available)
Src2-->>Service: Successful import (if fallback)
end
Service->>Logger: Log the count of loaded plugins
Possibly related PRs
Suggested reviewers
Poem
🪧 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 pause |
✅ Actions performedReviews paused. |
pujitm
force-pushed
the
refactor/config-defs
branch
2 times, most recently
from
c9b6b26 to
e592486
Compare
in order to simplify workspace bundling
pujitm
force-pushed
the
refactor/config-defs
branch
from
e592486 to
c9330be
Compare
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
pujitm
changed the title
|
This plugin has been deployed to Cloudflare R2 and is available for testing. |
![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 (4)
api/docs/developer/api-plugins.md (2)
5-5: Minor spelling suggestion.Consider using "bidirectional" (one word) instead of "bi-directional" for consistency with standard technical terminology.
-peer dependencies are installed by default as of npm v7, it supports bi-directional plugin functionality, +peer dependencies are installed by default as of npm v7, it supports bidirectional plugin functionality,🧰 Tools
🪛 LanguageTool
[misspelling] ~5-~5: This word is normally spelled as one.
Context: ...ed by default as of npm v7, it supports bi-directional plugin functionality, where the API pro...(EN_COMPOUNDS_BI_DIRECTIONAL)
13-13: Consider rewording for clarity.The phrase "we vendor them" might be clearer as "we include them" or "we package them".
-To solve this, we vendor them inside `dist/plugins`. To prevent the build from breaking, however, +To solve this, we include them inside `dist/plugins`. To prevent the build from breaking, however,🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Check that the noun ‘vendor’ after the pronoun ‘we’ is correct. It’s possible that you may need to switch to a possessive pronoun, or use another part of speech.
Context: ...a npm during production. To solve this, we vendor them insidedist/plugins. To prevent ...(PRP_VB)
api/scripts/copy-plugins.js (2)
1-60: Well-structured plugin build and copy scriptThis script effectively handles building and copying workspace plugins to the distribution folder. The implementation includes proper error handling, verification of build success, and clear logging.
Consider adding a cleanup step to remove old plugin distributions before copying new ones:
// Before line 31 (after finding workspace plugins) +// Clean up existing plugin directories +console.log('Cleaning up existing plugin directories...'); +for (const pkgName of workspacePlugins) { + const targetPath = path.resolve(pluginsDir, pkgName); + if (fs.existsSync(targetPath)) { + console.log(`Removing existing ${pkgName} directory...`); + fs.rmSync(targetPath, { recursive: true, force: true }); + } +}
39-47: Consider handling process termination more gracefullyWhile the error handling for build failures is good, a sudden process exit might not be ideal in all contexts.
} catch (error) { console.error(`Failed to build ${pkgName}:`, error.message); - process.exit(1); + throw new Error(`Build process for ${pkgName} failed`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
api/docs/developer/api-plugins.md(1 hunks)api/package.json(2 hunks)api/scripts/build.ts(2 hunks)api/scripts/copy-plugins.js(1 hunks)api/scripts/deploy-dev.sh(1 hunks)api/src/unraid-api/plugin/plugin.module.ts(1 hunks)api/src/unraid-api/plugin/plugin.service.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
api/src/unraid-api/plugin/plugin.service.ts (2)
api/scripts/copy-plugins.js (1)
pluginPrefix(26-26)api/src/environment.ts (1)
getPackageJson(36-42)
api/scripts/build.ts (2)
api/scripts/copy-plugins.js (1)
packageJson(17-17)api/scripts/get-deployment-version.ts (1)
getDeploymentVersion(13-34)
api/scripts/copy-plugins.js (1)
web/scripts/sync-webgui-repo.js (1)
path(2-2)
🪛 LanguageTool
api/docs/developer/api-plugins.md
[misspelling] ~5-~5: This word is normally spelled as one.
Context: ...ed by default as of npm v7, it supports bi-directional plugin functionality, where the API pro...
(EN_COMPOUNDS_BI_DIRECTIONAL)
[grammar] ~13-~13: Check that the noun ‘vendor’ after the pronoun ‘we’ is correct. It’s possible that you may need to switch to a possessive pronoun, or use another part of speech.
Context: ...a npm during production. To solve this, we vendor them inside dist/plugins. To prevent ...
(PRP_VB)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build and Deploy Plugin / Build and Deploy Plugin
- GitHub Check: Cloudflare Pages
🔇 Additional comments (15)
api/src/unraid-api/plugin/plugin.module.ts (1)
40-40: Change from log to debug level is appropriate.Reducing the log level from
logtodebugfor plugin discovery is a good practice. This change ensures that detailed plugin information is only shown in debug mode, while keeping production logs cleaner and more focused on important messages.api/scripts/deploy-dev.sh (1)
32-32: Added--deleteflag improves deployment synchronization.Adding the
--deleteflag to rsync ensures that the destination directory is an exact mirror of the source, removing any obsolete files that are no longer part of the build. This is particularly valuable for plugin management, as it will automatically clean up old plugin files during deployment.api/docs/developer/api-plugins.md (4)
1-7: Clear documentation on API plugin integration.The introduction effectively explains how API plugins are implemented as npm
peerDependencies, providing a solid foundation for understanding the plugin architecture.🧰 Tools
🪛 LanguageTool
[misspelling] ~5-~5: This word is normally spelled as one.
Context: ...ed by default as of npm v7, it supports bi-directional plugin functionality, where the API pro...(EN_COMPOUNDS_BI_DIRECTIONAL)
8-14: Clear explanation of workspace plugin challenges.This section effectively outlines the problem with local workspace plugins in production and introduces the solution approach.
🧰 Tools
🪛 LanguageTool
[grammar] ~13-~13: Check that the noun ‘vendor’ after the pronoun ‘we’ is correct. It’s possible that you may need to switch to a possessive pronoun, or use another part of speech.
Context: ...a npm during production. To solve this, we vendor them insidedist/plugins. To prevent ...(PRP_VB)
16-27: Excellent configuration example.The JSON example clearly demonstrates how to properly configure workspace dependencies as optional peer dependencies.
29-31: Good explanation of optional dependency behavior.This explains why marking the dependency as optional solves the potential build-time and run-time issues.
api/package.json (2)
23-23: Enhanced postbuild script supports plugin copying.Adding the plugin copying step to the postbuild script ensures that workspace plugins are properly included in the distribution.
139-146: Well-structured peer dependency configuration.The implementation of
unraid-api-plugin-connectas an optional peer dependency follows the pattern described in the documentation, making the code consistent with the guidance provided.api/src/unraid-api/plugin/plugin.service.ts (3)
51-65: Robust plugin import strategy with fallback mechanismsThe implementation now attempts to import plugins from multiple sources using
Promise.any, providing better resilience when handling both standard and workspace plugins. The detailed comments effectively explain the rationale behind this approach.
76-76: Good addition of logging for successful plugin countAdding this log statement improves observability of the plugin loading process, which will be helpful for monitoring and debugging.
84-89:❓ Verification inconclusive
Architectural shift from dependencies to peerDependencies
Changing from
dependenciestopeerDependenciesis aligned with modern plugin architecture best practices, making plugins optional and avoiding dependency duplication.
🏁 Script executed:
#!/bin/bash # Check if any plugin references still use dependencies instead of peerDependencies rg -A 3 -B 3 "\.dependencies" --type ts | grep -v "peerDependencies"Length of output: 68
Action: Verify Peer Dependency Shift in Plugin Service
The update in
api/src/unraid-api/plugin/plugin.service.ts(lines 84–89) correctly shifts from usingdependenciestopeerDependencies, aligning with modern plugin architecture best practices. The change ensures that plugins are optional and prevents dependency duplication.
- The code now extracts
peerDependenciesfromgetPackageJson()and logs a warning if they are missing.- The automated script did not produce output, so please manually verify that no outdated references to
dependenciesremain in the repository.api/scripts/build.ts (4)
2-2: Simplified importsRemoving unused imports keeps the code clean and focused.
5-5: Improved type safety with custom ApiPackageJson typeThe new type alias enhances type safety by explicitly defining required properties of the package.json.
Also applies to: 10-13
28-28: Type assertion updated for package.json parsingThis change properly applies the new ApiPackageJson type to the parsed JSON.
33-33: Clarified comment about dependency handlingThe updated comment provides better context about the purpose of emptying devDependencies.
## Summary by CodeRabbit - **New Features** - Introduced an automated step in the post-build process to copy plugin assets. - Enhanced the plugin import process by supporting multiple sourcing options. - Adds a demo `health` query via a workspace plugin. - **Documentation** - Added a detailed guide explaining API plugin configuration and local workspace integration. - **Refactor** - Improved dependency handling by marking certain workspace plugins as optional. - Updated deployment synchronization to ensure destination directories exactly mirror the source. - Refined logging levels and type-safety for improved reliability and debugging.
Summary by CodeRabbit
New Features
healthquery via a workspace plugin.Documentation
Refactor