Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions .github/workflows/build-and-publish.yml
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,42 @@ jobs:
- name: Install dependencies
run: |
echo "Installing dependencies..."
if [ -f "package-lock.json" ]; then
npm ci
else
npm install
fi
echo "Dependencies installed successfully"

# Retry function for npm install
install_with_retry() {
local max_attempts=3
local attempt=1
local delay=5

while [ $attempt -le $max_attempts ]; do
echo "Attempt $attempt of $max_attempts..."

if [ -f "package-lock.json" ]; then
if npm ci; then
echo "✅ Dependencies installed successfully"
return 0
fi
else
if npm install; then
echo "✅ Dependencies installed successfully"
return 0
fi
fi

if [ $attempt -lt $max_attempts ]; then
echo "⚠️ Installation failed, retrying in ${delay} seconds..."
sleep $delay
delay=$((delay * 2)) # Exponential backoff
fi

attempt=$((attempt + 1))
done

echo "❌ Failed to install dependencies after $max_attempts attempts"
return 1
}

install_with_retry

- name: Validate JSON schemas
run: |
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"build": "echo 'Build completed - package is ready'",
"sync-schema": "node sync-schema-version.js",
"setup": "node setup.js",
"postinstall": "node setup.js && npm run sync-schema",
"postinstall": "(node setup.js || true) && npm run sync-schema",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The use of || true will suppress any exit code from setup.js, causing the postinstall step to always appear successful even if the setup fails for a legitimate reason (e.g., an invalid domain name provided by a user in an interactive session). This can be misleading as it hides failures. Since setup.js is being modified to exit gracefully in non-interactive environments, this error suppression might not be necessary and could be detrimental in interactive environments. Consider removing || true and ensuring setup.js has the correct exit behavior for all scenarios.

Suggested change
"postinstall": "(node setup.js || true) && npm run sync-schema",
"postinstall": "node setup.js && npm run sync-schema",

"prepublishOnly": "npm run sync-schema && npm run validate"
},
"bin": {
Expand Down
30 changes: 25 additions & 5 deletions setup.js
Original file line number Diff line number Diff line change
Expand Up @@ -185,9 +185,13 @@ function isInstalledAsDependency() {
return cwd.includes('node_modules');
}

// Check if running in non-interactive environment (like npm install)
// Check if running in non-interactive environment (like npm install or CI)
function isNonInteractive() {
return !process.stdin.isTTY || process.env.CI === 'true' || process.env.NPM_CONFIG_INTERACTIVE === 'false';
return !process.stdin.isTTY ||
process.env.CI === 'true' ||
process.env.GITHUB_ACTIONS === 'true' ||
process.env.NPM_CONFIG_INTERACTIVE === 'false' ||
process.env.CI === '1';
Comment on lines +190 to +194

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for detecting a non-interactive environment can be simplified. The CI environment variable is set to 'true' in GitHub Actions, so checking for process.env.GITHUB_ACTIONS is redundant. Also, checking for CI being 'true' or '1' can be simplified to just checking for its presence, which is a common and more robust practice. This makes the code cleaner and easier to maintain.

Suggested change
return !process.stdin.isTTY ||
process.env.CI === 'true' ||
process.env.GITHUB_ACTIONS === 'true' ||
process.env.NPM_CONFIG_INTERACTIVE === 'false' ||
process.env.CI === '1';
return !process.stdin.isTTY ||
!!process.env.CI ||
process.env.NPM_CONFIG_INTERACTIVE === 'false';

}

// Get domain name from vnext.config.json
Expand Down Expand Up @@ -232,18 +236,34 @@ async function setup() {
return;
}

console.log('🚀 vNext Template Setup');
console.log('=======================\n');

// Check if domain folder from vnext.config.json exists
const domainFromConfig = getDomainFromConfig();
if (domainFromConfig && isDomainFolderConfigured()) {
// In CI/non-interactive mode, skip silently
if (isNonInteractive()) {
return;
}
// In interactive mode, show message
console.log('🚀 vNext Template Setup');
console.log('=======================\n');
console.log(`✅ Domain "${domainFromConfig}" is already configured`);
console.log(` Domain folder "${domainFromConfig}" exists and is set up.`);
console.log(' Skipping setup. If you want to re-run setup, remove the domain directory first.\n');
return;
Comment on lines +243 to 252
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (bug_risk): The touch file existence check is currently a no-op due to the unconditional return.

In the isNonInteractive() block, this code:

if (!fs.existsSync('touch')) {
  return;
}
return;

returns in all cases, so the fs.existsSync('touch') check and its comment are misleading and effectively dead logic. If touch is supposed to change behavior, the second unconditional return needs adjusting; if not, this can be simplified to if (isNonInteractive()) return; to match the actual behavior.

}

// In CI/non-interactive mode, if touch doesn't exist or not configured, skip silently
if (isNonInteractive()) {
if (!fs.existsSync('touch')) {
return;
}
// If touch exists but we're in CI, skip silently (don't prompt)
return;
}

console.log('🚀 vNext Template Setup');
console.log('=======================\n');
Comment on lines 241 to +265

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This section has some duplicated code and complex logic. The header is printed in two separate places, and the check for non-interactive mode is split and contains a redundant conditional. This can be refactored to be more readable and maintainable by handling the non-interactive case upfront, and then printing the header once for all interactive scenarios.

  if (isNonInteractive()) {
    // In non-interactive mode (e.g. CI), we should not perform any setup
    // or display any output. The script should exit gracefully.
    return;
  }

  // Interactive setup starts here
  console.log('🚀 vNext Template Setup');
  console.log('=======================\n');

  if (domainFromConfig && isDomainFolderConfigured()) {
    console.log(`✅ Domain "${domainFromConfig}" is already configured`);
    console.log(`   Domain folder "${domainFromConfig}" exists and is set up.`);
    console.log('   Skipping setup. If you want to re-run setup, remove the domain directory first.\n');
    return;
  }


// Check if already set up
if (isAlreadySetup()) {
console.log('✅ Template is already set up with a domain name');
Expand Down
Loading