fix: copy Composer runtime files for premium build#59
Open
Arukuen wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthrough
ChangesPremium Composer Runtime Packaging
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
scripts/package.js (3)
335-342: 💤 Low valueConsider moving the log statement inside the function for consistency.
The current pattern has conditional logging followed by an unconditional function call (which has an internal guard). Other similar functions in this file (
addSecurityFiles,updatePluginHeaderVersion) are called unconditionally without conditional logging wrappers. For consistency, either move the logging insidecopyPremiumComposerRuntime, or wrap the entire function call in theifblock.♻️ Option 1: Move logging inside the function
- // Copy Composer runtime files for premium build - // Only do this in premium since the SVG sanitizer used - // for SVG optimization is a premium-only feature. - if ( IS_PREMIUM_BUILD ) { - // eslint-disable-next-line no-console - console.log( '📁 Copying Composer runtime files...' ) - } copyPremiumComposerRuntime( BUILD_DIR )Then inside
copyPremiumComposerRuntime(after line 195):function copyPremiumComposerRuntime( buildDir ) { if ( ! IS_PREMIUM_BUILD ) { return } + // Copy Composer runtime files for premium build + // Only do this in premium since the SVG sanitizer used + // for SVG optimization is a premium-only feature. + // eslint-disable-next-line no-console + console.log( '📁 Copying Composer runtime files...' ) for ( const runtimePath of PREMIUM_COMPOSER_RUNTIME_PATHS ) {🤖 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 `@scripts/package.js` around lines 335 - 342, The console.log belongs inside copyPremiumComposerRuntime for consistency: remove the external IS_PREMIUM_BUILD conditional logging and instead add the '📁 Copying Composer runtime files...' log at the start of the copyPremiumComposerRuntime function (immediately after its internal premium guard), so callers can call copyPremiumComposerRuntime(BUILD_DIR) unconditionally (matching patterns used by addSecurityFiles and updatePluginHeaderVersion) while the function itself handles whether to log and act.
199-201: ⚡ Quick winImprove error message with actionable guidance.
When Composer runtime files are missing, guide the user to install dependencies.
📝 Suggested improvement
throw new Error( - `Missing Composer runtime file required for premium build: ${ runtimePath }` + `Missing Composer runtime file required for premium build: ${ runtimePath }. Run 'composer install' to install dependencies.` )🤖 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 `@scripts/package.js` around lines 199 - 201, Update the thrown Error in scripts/package.js that currently says `Missing Composer runtime file required for premium build: ${ runtimePath }` to include actionable guidance: mention which dependency or package to install (e.g., run npm/yarn install or install the Composer runtime package), provide the command(s) to run, and suggest verifying the runtimePath or re-running the build; change the message where the Error is constructed (the throw new Error(...) that references runtimePath) to include these instructions so users know how to resolve the missing Composer runtime files.
208-208: ⚖️ Poor tradeoffConsider using a vendor-specific copy function instead of
copyDir.
copyDir(lines 84-146) excludes.js,.css,.map,.md,package.json, andnode_modulesbecause it was designed for WordPress plugin source directories. For Composer vendor directories, these exclusions may be too aggressive. While the currentenshrined/svg-sanitizelibrary is pure PHP and won't be affected, future Composer dependencies might include runtime-required assets (e.g., browser-side JS validators, CSS for admin UI components) that would be silently excluded.Consider either:
- Using
copyBuiltDir(which copies everything without exclusions), or- Creating a dedicated
copyVendorDirfunction with vendor-appropriate exclusions (e.g., only exclude tests, docs, dev tools).♻️ Option 1: Use copyBuiltDir for vendor directories
if ( stat.isDirectory() ) { - copyDir( runtimePath, destPath ) + copyBuiltDir( runtimePath, destPath ) } else {🤖 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 `@scripts/package.js` at line 208, The current call to copyDir(runtimePath, destPath) uses a function that intentionally excludes files like .js, .css, .map, .md, package.json and node_modules (see copyDir), which is too aggressive for Composer/vendor packages; replace this call with copyBuiltDir(runtimePath, destPath) to copy everything, or implement a new copyVendorDir(source, dest) that only excludes dev artifacts (tests, docs, examples) but preserves runtime assets (JS/CSS/package.json), and then call that from where copyDir is currently invoked.
🤖 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.
Nitpick comments:
In `@scripts/package.js`:
- Around line 335-342: The console.log belongs inside copyPremiumComposerRuntime
for consistency: remove the external IS_PREMIUM_BUILD conditional logging and
instead add the '📁 Copying Composer runtime files...' log at the start of the
copyPremiumComposerRuntime function (immediately after its internal premium
guard), so callers can call copyPremiumComposerRuntime(BUILD_DIR)
unconditionally (matching patterns used by addSecurityFiles and
updatePluginHeaderVersion) while the function itself handles whether to log and
act.
- Around line 199-201: Update the thrown Error in scripts/package.js that
currently says `Missing Composer runtime file required for premium build: ${
runtimePath }` to include actionable guidance: mention which dependency or
package to install (e.g., run npm/yarn install or install the Composer runtime
package), provide the command(s) to run, and suggest verifying the runtimePath
or re-running the build; change the message where the Error is constructed (the
throw new Error(...) that references runtimePath) to include these instructions
so users know how to resolve the missing Composer runtime files.
- Line 208: The current call to copyDir(runtimePath, destPath) uses a function
that intentionally excludes files like .js, .css, .map, .md, package.json and
node_modules (see copyDir), which is too aggressive for Composer/vendor
packages; replace this call with copyBuiltDir(runtimePath, destPath) to copy
everything, or implement a new copyVendorDir(source, dest) that only excludes
dev artifacts (tests, docs, examples) but preserves runtime assets
(JS/CSS/package.json), and then call that from where copyDir is currently
invoked.
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.
fixes #55
Summary by CodeRabbit