Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chore: Improve varous aspects of sandbox creation #28965

Open
wants to merge 6 commits into
base: next
Choose a base branch
from
Open
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
25 changes: 13 additions & 12 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ Thank you for contributing to Storybook! Please submit all PRs to the `next` bra
<!-- Please check (put an "x" inside the "[ ]") the applicable items below to communicate how to test your changes -->

#### The changes in this PR are covered in the following automated tests:

- [ ] stories
- [ ] unit tests
- [ ] integration tests
Expand Down Expand Up @@ -46,21 +47,21 @@ _This section is mandatory for all contributions. If you believe no manual test

## Checklist for Maintainers

- [ ] When this PR is ready for testing, make sure to add `ci:normal`, `ci:merged` or `ci:daily` GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in `code/lib/cli/src/sandbox-templates.ts`
- [ ] When this PR is ready for testing, make sure to add `ci:normal`, `ci:merged` or `ci:daily` GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in `code/lib/cli-storybook/src/sandbox-templates.ts`
- [ ] Make sure this PR contains **one** of the labels below:
<details>
<summary>Available labels</summary>

- `bug`: Internal changes that fixes incorrect behavior.
- `maintenance`: User-facing maintenance tasks.
- `dependencies`: Upgrading (sometimes downgrading) dependencies.
- `build`: Internal-facing build tooling & test updates. Will not show up in release changelog.
- `cleanup`: Minor cleanup style change. Will not show up in release changelog.
- `documentation`: Documentation **only** changes. Will not show up in release changelog.
- `feature request`: Introducing a new feature.
- `BREAKING CHANGE`: Changes that break compatibility in some way with current major version.
- `other`: Changes that don't fit in the above categories.
- `bug`: Internal changes that fixes incorrect behavior.
- `maintenance`: User-facing maintenance tasks.
- `dependencies`: Upgrading (sometimes downgrading) dependencies.
- `build`: Internal-facing build tooling & test updates. Will not show up in release changelog.
- `cleanup`: Minor cleanup style change. Will not show up in release changelog.
- `documentation`: Documentation **only** changes. Will not show up in release changelog.
- `feature request`: Introducing a new feature.
- `BREAKING CHANGE`: Changes that break compatibility in some way with current major version.
- `other`: Changes that don't fit in the above categories.

</details>

### 🦋 Canary release
Expand All @@ -74,4 +75,4 @@ _core team members can create a canary release [here](https://github.com/storybo
<!-- CANARY_RELEASE_SECTION -->

<!-- BENCHMARK_SECTION -->
<!-- BENCHMARK_SECTION -->
<!-- BENCHMARK_SECTION -->
2 changes: 1 addition & 1 deletion docs/contribute/code.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,7 @@ npx storybook@next link --local /path/to/local-repro-directory

## Developing a template

The first step is to add an entry to `code/lib/cli/src/sandbox-templates.ts`, which is the master list of all repro templates:
The first step is to add an entry to `code/lib/cli-storybook/src/sandbox-templates.ts`, which is the master list of all repro templates:

```ts
'cra/default-js': {
Expand Down
18 changes: 15 additions & 3 deletions scripts/sandbox/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as ghActions from '@actions/core';
import { program } from 'commander';
import type { Options as ExecaOptions } from 'execa';
import { execaCommand } from 'execa';
import { copy, emptyDir, ensureDir, move, remove, rename, writeFile } from 'fs-extra';
import { copy, emptyDir, ensureDir, move, remove, writeFile } from 'fs-extra';
import pLimit from 'p-limit';
import { join, relative } from 'path';
import prettyTime from 'pretty-hrtime';
Expand Down Expand Up @@ -126,7 +126,7 @@ const addStorybook = async ({
throw e;
}

await rename(tmpDir, afterDir);
await copy(tmpDir, afterDir);
};

export const runCommand = async (script: string, options: ExecaOptions, debug = false) => {
Expand Down Expand Up @@ -194,7 +194,19 @@ const runGenerators = async (
// We do the creation inside a temp dir to avoid yarn container problems
const createBaseDir = await temporaryDirectory();
if (!script.includes('pnp')) {
await setupYarn({ cwd: createBaseDir });
try {
await setupYarn({ cwd: createBaseDir });
} catch (error) {
const message = `❌ Failed to setup yarn in template: ${name} (${dirName})`;
if (isCI) {
ghActions.error(dedent`${message}
${(error as any).stack}`);
} else {
console.error(message);
console.error(error);
}
throw new Error(message);
}
}

const createBeforeDir = join(createBaseDir, BEFORE_DIR_NAME);
Expand Down
5 changes: 3 additions & 2 deletions scripts/sandbox/utils/yarn.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import fs from 'fs';
import { move, remove } from 'fs-extra';
import { join } from 'path';

Expand All @@ -11,7 +12,7 @@ interface SetupYarnOptions {

export async function setupYarn({ cwd, pnp = false, version = 'classic' }: SetupYarnOptions) {
// force yarn
await runCommand(`touch yarn.lock`, { cwd });
fs.writeFileSync(join(cwd, 'yarn.lock'), '', { flag: 'a' });
await runCommand(`yarn set version ${version}`, { cwd });
if (version === 'berry' && !pnp) {
await runCommand('yarn config set nodeLinker node-modules', { cwd });
Expand All @@ -21,7 +22,7 @@ export async function setupYarn({ cwd, pnp = false, version = 'classic' }: Setup

export async function localizeYarnConfigFiles(baseDir: string, beforeDir: string) {
await Promise.allSettled([
runCommand(`touch yarn.lock`, { cwd: beforeDir }),
fs.writeFileSync(join(beforeDir, 'yarn.lock'), '', { flag: 'a' }),
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Ensure this operation is wrapped in a try-catch block to handle potential file system errors

move(join(baseDir, '.yarn'), join(beforeDir, '.yarn')),
move(join(baseDir, '.yarnrc.yml'), join(beforeDir, '.yarnrc.yml')),
move(join(baseDir, '.yarnrc'), join(beforeDir, '.yarnrc')),
Expand Down
2 changes: 1 addition & 1 deletion scripts/utils/yarn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ export const configureYarn2ForVerdaccio = async ({
// ⚠️ Need to set registry because Yarn 2 is not using the conf of Yarn 1 (URL is hardcoded in CircleCI config.yml)
`yarn config set npmRegistryServer "http://localhost:6001/"`,
// Some required magic to be able to fetch deps from local registry
`yarn config set unsafeHttpWhitelist --json '["localhost"]'`,
`yarn config set unsafeHttpWhitelist "localhost"`,
// Disable fallback mode to make sure everything is required correctly
`yarn config set pnpFallbackMode none`,
// We need to be able to update lockfile when bootstrapping the examples
Expand Down