chore(scripts): improve bootstrap.js with npm fallback and clearer errors#51
chore(scripts): improve bootstrap.js with npm fallback and clearer errors#51itspavant wants to merge 1 commit intocloudinary:masterfrom
Conversation
|
Dear Maintainers, Please review the PR and let me know if you need any more modifications Thank you. |
There was a problem hiding this comment.
Thanks for taking the time to improve the bootstrap experience! 👍 I appreciate the effort to support both yarn and npm users with better error messages.
However, I have some concerns about the current approach:
Complexity
The script has grown from ~29 lines to ~132 lines (4.5x increase). While the npm fallback is valuable, this adds significant maintenance burden for what should be a simple bootstrap script.
Potential Logic Issue
There appears to be a bug in the fallback logic. After the yarn bootstrap attempt, this block runs:
// package.json.bootstrap = "yarn example && yarn install"
// npm-equivalent:
// 1) npm --prefix ./example install
// 2) npm install (root)
// run step 1
const step1 = runSync('npm', ['--prefix', 'example', 'install'], options);
if (step1.status !== 0) {
console.error('Failed to install dependencies in ./example using npm.');
process.exit(step1.status);
}
// run step 2
const step2 = runSync('npm', ['install'], options);
This block will execute whenever hasNpm is true, even if yarn bootstrap succeeded. This means both yarn AND npm could run installations, which is wasteful and could cause issues.
Also:
// run yarn bootstrap
result = runSync('yarn', ['bootstrap'], options);
// If yarn bootstrap fails but npm exists, try npm equivalent as fallback
if (result.status !== 0 && hasNpm) {
console.warn('yarn bootstrap failed; attempting npm-equivalent bootstrap...');
// fall through to npm-equivalent
}
If yarn fails for a legitimate reason (network issues, package conflicts, etc.), silently falling back to npm could mask the real problem and make debugging harder.
Brief Summary of Changes
Improved
scripts/bootstrap.jsdeveloper experience:yarnis missing and provides a clear error message instead of failing silently.yarn bootstrapstill runs as before).bootstrap.jsto support both Yarn and NPM with clearer errors #50What does this PR address?
Are tests included?
(The setup for this repo is non-trivial, so I did not run the full suite. This PR only modifies
scripts/bootstrap.js, which is used for contributor setup, so it does not impact SDK runtime or published package behavior.)Reviewer, please note:
package.jsoncurrently defines"bootstrap": "yarn example && yarn install".If Yarn is not installed, the updated script translates this to equivalent npm commands:
npm --prefix example installnpm install(root)Checklist:
(Not run due to complexity, but the change is isolated to the bootstrap script and does not affect published code.)