Conversation
The postinstall script (patch-package) runs when consumers install
@bsf/force-ui as a git dependency, but patch-package was only in
devDependencies, so consumer installs failed with:
sh: patch-package: command not found (exit 127)
Moving it to dependencies fixes the install and also ensures the
Lexical Shadow DOM patches get applied to the host project's
node_modules — required at runtime because the build externalizes
lexical, so consumers would otherwise run unpatched lexical and the
Shadow DOM mention fix would silently not work.
…release artifacts v1.7.12 consumer installs failed with 'patch-package: command not found' because the postinstall ran in the host project where patch-package (a devDependency) isn't installed. Moving it to dependencies fixed the crash but not the root problem: patch-package resolved node_modules relative to the installed package dir, while npm hoists lexical to the host root, so the runtime-critical Shadow DOM patches never applied. - Add apply-patches.cjs postinstall wrapper that detects consumer installs and runs patch-package from the host project root with --patch-dir pointing at this package's patches. Lives at the repo root because .distignore strips bin/ from release tags. - Add files field so npm publish includes dist (previously excluded by .gitignore, publishing a package with no build output), patches, and the postinstall script. - Ship patches and apply-patches.cjs in the force-ui.zip archive and in the bsf-admin-ui public mirror, syncing the mirror's postinstall script and patch-package dependency. - Bump version to 1.7.13 and add changelog entry. Verified: fresh tarball install applies all 6 patches to the host's lexical (exit 0); dev checkout postinstall unchanged; release.sh rsync simulation confirms apply-patches.cjs, dist/ and patches/ reach the release tag.
…tall fix: `patch-package` install failure for consumers + ship Lexical patches in all release artifacts (`v1.7.13`)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| * 1. Local development checkout — patches apply to this package's own | ||
| * node_modules. | ||
| * 2. Installed as a dependency of a host project — npm hoists lexical to the | ||
| * host's node_modules, so patch-package must run from the host project |
There was a problem hiding this comment.
What: Consider adding error handling for filesystem operations.
Why: While the current implementation will exit gracefully if the patch directory does not exist or if 'patch-package' is unavailable, it lacks detailed feedback. Proper error handling can help in understanding the failure points during execution.
How: Use console.error() to log specific error messages before process.exit. This can provide context on issues for future debugging.
| } | ||
|
|
||
| const segments = pkgDir.split( path.sep ); | ||
| const nmIndex = segments.lastIndexOf( 'node_modules' ); |
There was a problem hiding this comment.
What: Validate the existence of the patch binary before attempting to execute it.
Why: Directly constructing the path to the 'patch-package' binary without validating its existence increases the chance of runtime errors. This would enhance the robustness of the script.
How: Add a check after defining patchPackageBin to ensure the file exists using fs.existsSync(patchPackageBin). If it does not exist, log an error and exit.
| } ); | ||
| const manifest = JSON.parse( fs.readFileSync( manifestPath, 'utf8' ) ); | ||
| const binEntry = | ||
| typeof manifest.bin === 'string' |
There was a problem hiding this comment.
What: Consider using spawn with proper callbacks instead of spawnSync.
Why: Using synchronous calls might block the event loop, especially if this script is executed in a larger context, which could lead to performance bottlenecks.
How: Switch to using spawn from 'child_process', and implement appropriate callback functions to handle output and errors asynchronously.
| process.execPath, | ||
| [ patchPackageBin, '--patch-dir', path.relative( appRoot, patchDir ) ], | ||
| { cwd: appRoot, stdio: 'inherit' } | ||
| ); |
There was a problem hiding this comment.
What: Ensure the exit status checks are handled properly after executing the patch command.
Why: While the current implementation checks the result of the command execution, it is beneficial to provide specific behavior based on different exit statuses (e.g., 0 for success, others for failure) to offer better context.
How: Instead of exiting with a direct mapping to result.status, consider logging a message about the success or failure of the command and correlate it with specific exit codes for clarity.
| @@ -1,12 +1,12 @@ | |||
| { | |||
There was a problem hiding this comment.
What: Consider updating the lockfile version to the latest compatible version if possible for better compatibility and performance improvements.
Why: Using the latest lockfile version can improve performance and future compatibility with npm or yarn. It's important to stay up to date with package management tools to benefit from optimizations and security fixes.
How: Check the npm documentation for the latest lockfile version and update accordingly.
| "name": "@bsf/force-ui", | ||
| "version": "1.7.12", | ||
| "version": "1.7.13", | ||
| "lockfileVersion": 3, |
There was a problem hiding this comment.
What: The addition of patch-package is noted; ensure that it is appropriately integrated into your build process if changes to dependencies are being modified.
Why: Without proper configuration, changes to dependencies might not be effective, which can lead to inconsistencies across environments if the patched changes are not applied.
How: Review the documentation for patch-package to ensure it’s being utilized correctly in your build or development processes.
| @@ -1,12 +1,12 @@ | |||
| { | |||
| "name": "@bsf/force-ui", | |||
There was a problem hiding this comment.
What: Multiple ‘dev’ properties have been removed, may consider marking certain packages in dev dependencies explicitly if they are development tools.
Why: Excessively listing unnecessary dev dependencies can lead to larger package sizes in production, and mismanagement of dev dependencies can complicate deployment.
How: Categorize dependencies appropriately in your package.json file, ensuring that development only needed dependencies are clearly marked.
| "version": "1.7.12", | ||
| "version": "1.7.13", | ||
| "description": "Library of components for the BSF project", | ||
| "main": "./dist/force-ui.cjs.js", |
There was a problem hiding this comment.
What: The files array has been added, which exposes certain directories and files during packaging. Ensure that sensitive files are not unintentionally exposed.
Why: Exposing unnecessary files can lead to security vulnerabilities and may also confuse users or developers who install the package.
How: Review the contents of dist, patches, and apply-patches.cjs to verify that they contain only the necessary components for your package. If there are sensitive files or development artifacts, remove them from this list.
| "types": "./dist/force-ui.d.ts", | ||
| "type": "module", | ||
| "sideEffects": false, | ||
| "files": [ |
There was a problem hiding this comment.
What: The postinstall command has been changed to use node apply-patches.cjs. Ensure this script functions correctly without exposing vulnerabilities.
Why: The postinstall script runs automatically after package installation, which raises concerns about code execution integrity. If the script is compromised, it could lead to security breaches.
How: Make sure that apply-patches.cjs is secure, and consider adding security checks to the script to validate its integrity before execution. Additionally, document how to disable or modify this script if needed.
| "type": "module", | ||
| "sideEffects": false, | ||
| "files": [ | ||
| "dist", |
There was a problem hiding this comment.
What: Confirm that dependencies are accurate and necessary. Only include patch-package if it is actively used.
Why: Excess dependencies increase the attack surface and may introduce vulnerabilities or performance issues.
How: Review the usage of each dependency and consider if any can be removed or replaced with lighter alternatives. Check if patch-package is truly needed and if it is the most secure option.
| @@ -1,3 +1,3 @@ | |||
| { | |||
| "force-ui": "1.7.12" | |||
There was a problem hiding this comment.
What: The version number is updated correctly, but there are no accompanying details or comments about changes in this version.
Why: Providing a changelog or a summary of changes ensures that developers and users understand what improvements or fixes are introduced in this version, aiding in debugging and tracking issues.
How: Include a section in the PR description that summarizes changes from version 1.7.12 to 1.7.13 and ensure this information is documented in the version.json as a comments field or in a separate CHANGELOG file.
2 participants
Description
Screenshots | Video with voice-over
Link to Figma (If applicable)
How has this been tested?
Checklist: