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

CLI: Fix the initialization of Storybook in workspaces #28699

Merged
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
18 changes: 2 additions & 16 deletions code/core/src/common/js-package-manager/JsPackageManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,22 +64,6 @@ export abstract class JsPackageManager {
return packageJSON ? packageJSON.version ?? null : null;
}

// NOTE: for some reason yarn prefers the npm registry in
// local development, so always use npm
async setRegistryURL(url: string) {
if (url) {
await this.executeCommand({ command: 'npm', args: ['config', 'set', 'registry', url] });
} else {
await this.executeCommand({ command: 'npm', args: ['config', 'delete', 'registry'] });
}
}

async getRegistryURL() {
const res = await this.executeCommand({ command: 'npm', args: ['config', 'get', 'registry'] });
const url = res.trim();
return url === 'undefined' ? undefined : url;
}

constructor(options?: JsPackageManagerOptions) {
this.cwd = options?.cwd || process.cwd();
}
Expand Down Expand Up @@ -487,6 +471,8 @@ export abstract class JsPackageManager {
): // Use generic and conditional type to force `string[]` if fetchAllVersions is true and `string` if false
Promise<T extends true ? string[] : string>;

public abstract getRegistryURL(): Promise<string | undefined>;

public abstract runPackageCommand(
command: string,
args: string[],
Expand Down
15 changes: 0 additions & 15 deletions code/core/src/common/js-package-manager/NPMProxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,21 +34,6 @@ describe('NPM Proxy', () => {
});
});

describe('setRegistryUrl', () => {
it('should run `npm config set registry https://foo.bar`', async () => {
const executeCommandSpy = vi.spyOn(npmProxy, 'executeCommand').mockResolvedValueOnce('');

await npmProxy.setRegistryURL('https://foo.bar');

expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['config', 'set', 'registry', 'https://foo.bar'],
})
);
});
});

describe('installDependencies', () => {
describe('npm6', () => {
it('should run `npm install`', async () => {
Expand Down
11 changes: 11 additions & 0 deletions code/core/src/common/js-package-manager/NPMProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,17 @@ export class NPMProxy extends JsPackageManager {
});
}

public async getRegistryURL() {
const res = await this.executeCommand({
command: 'npm',
// "npm config" commands are not allowed in workspaces per default
// https://github.com/npm/cli/issues/6099#issuecomment-1847584792
args: ['config', 'get', 'registry', '-ws=false', '-iwr'],
});
const url = res.trim();
return url === 'undefined' ? undefined : url;
}

protected async runAddDeps(dependencies: string[], installAsDevDependencies: boolean) {
const { logStream, readLogFile, moveLogFile, removeLogFile } = await createLogStream();
let args = [...dependencies];
Expand Down
15 changes: 0 additions & 15 deletions code/core/src/common/js-package-manager/PNPMProxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,6 @@ describe('PNPM Proxy', () => {
});
});

describe('setRegistryUrl', () => {
it('should run `npm config set registry https://foo.bar`', async () => {
const executeCommandSpy = vi.spyOn(pnpmProxy, 'executeCommand').mockResolvedValueOnce('');

await pnpmProxy.setRegistryURL('https://foo.bar');

expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['config', 'set', 'registry', 'https://foo.bar'],
})
);
});
});

describe('installDependencies', () => {
it('should run `pnpm install`', async () => {
const executeCommandSpy = vi
Expand Down
9 changes: 9 additions & 0 deletions code/core/src/common/js-package-manager/PNPMProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,15 @@ export class PNPMProxy extends JsPackageManager {
});
}

public async getRegistryURL() {
const res = await this.executeCommand({
command: 'pnpm',
args: ['config', 'get', 'registry'],
});
const url = res.trim();
return url === 'undefined' ? undefined : url;
}

async runPackageCommand(command: string, args: string[], cwd?: string): Promise<string> {
return this.executeCommand({
command: 'pnpm',
Expand Down
15 changes: 0 additions & 15 deletions code/core/src/common/js-package-manager/Yarn1Proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,21 +25,6 @@ describe('Yarn 1 Proxy', () => {
});
});

describe('setRegistryUrl', () => {
it('should run `yarn config set npmRegistryServer https://foo.bar`', async () => {
const executeCommandSpy = vi.spyOn(yarn1Proxy, 'executeCommand').mockResolvedValueOnce('');

await yarn1Proxy.setRegistryURL('https://foo.bar');

expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['config', 'set', 'registry', 'https://foo.bar'],
})
);
});
});

describe('installDependencies', () => {
it('should run `yarn`', async () => {
const executeCommandSpy = vi.spyOn(yarn1Proxy, 'executeCommand').mockResolvedValueOnce('');
Expand Down
9 changes: 9 additions & 0 deletions code/core/src/common/js-package-manager/Yarn1Proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ export class Yarn1Proxy extends JsPackageManager {
return JSON.parse(readFileSync(packageJsonPath, 'utf-8')) as Record<string, any>;
}

public async getRegistryURL() {
const res = await this.executeCommand({
command: 'yarn',
args: ['config', 'get', 'registry'],
});
const url = res.trim();
return url === 'undefined' ? undefined : url;
}

public async findInstallations(pattern: string[], { depth = 99 }: { depth?: number } = {}) {
const yarnArgs = ['list', '--pattern', pattern.map((p) => `"${p}"`).join(' '), '--json'];

Expand Down
15 changes: 0 additions & 15 deletions code/core/src/common/js-package-manager/Yarn2Proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,21 +53,6 @@ describe('Yarn 2 Proxy', () => {
});
});

describe('setRegistryUrl', () => {
it('should run `yarn config set npmRegistryServer https://foo.bar`', async () => {
const executeCommandSpy = vi.spyOn(yarn2Proxy, 'executeCommand').mockResolvedValueOnce('');

await yarn2Proxy.setRegistryURL('https://foo.bar');

expect(executeCommandSpy).toHaveBeenCalledWith(
expect.objectContaining({
command: 'npm',
args: ['config', 'set', 'registry', 'https://foo.bar'],
})
);
});
});

describe('addDependencies', () => {
it('with devDep it should run `yarn install -D @storybook/core`', async () => {
const executeCommandSpy = vi.spyOn(yarn2Proxy, 'executeCommand').mockResolvedValueOnce('');
Expand Down
9 changes: 9 additions & 0 deletions code/core/src/common/js-package-manager/Yarn2Proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,15 @@ export class Yarn2Proxy extends JsPackageManager {
await removeLogFile();
}

public async getRegistryURL() {
const res = await this.executeCommand({
command: 'yarn',
args: ['config', 'get', 'npmRegistryServer'],
});
const url = res.trim();
return url === 'undefined' ? undefined : url;
}

protected async runRemoveDeps(dependencies: string[]) {
const args = [...dependencies];

Expand Down
46 changes: 34 additions & 12 deletions scripts/sandbox/generate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,18 +45,34 @@ const sbInit = async (
await runCommand(`${sbCliBinaryPath} init ${fullFlags.join(' ')}`, { cwd, env }, debug);
};

const withLocalRegistry = async (packageManager: JsPackageManager, action: () => Promise<void>) => {
type LocalRegistryProps = {
packageManager: JsPackageManager;
action: () => Promise<void>;
cwd: string;
env: Record<string, any>;
debug: boolean;
};

const withLocalRegistry = async ({
packageManager,
action,
cwd,
env,
debug,
}: LocalRegistryProps) => {
const prevUrl = await packageManager.getRegistryURL();
let error;
try {
console.log(`📦 Configuring local registry: ${LOCAL_REGISTRY_URL}`);
packageManager.setRegistryURL(LOCAL_REGISTRY_URL);
// NOTE: for some reason yarn prefers the npm registry in
// local development, so always use npm
await runCommand(`npm config set registry ${LOCAL_REGISTRY_URL}`, { cwd, env }, debug);
await action();
} catch (e) {
error = e;
} finally {
console.log(`📦 Restoring registry: ${prevUrl}`);
await packageManager.setRegistryURL(prevUrl);
await runCommand(`npm config set registry ${prevUrl}`, { cwd, env }, debug);

if (error) {
throw error;
Expand Down Expand Up @@ -88,14 +104,20 @@ const addStorybook = async ({

const packageManager = JsPackageManagerFactory.getPackageManager({ force: 'yarn1' }, tmpDir);
if (localRegistry) {
await withLocalRegistry(packageManager, async () => {
await packageManager.addPackageResolutions({
...storybookVersions,
// Yarn1 Issue: https://github.com/storybookjs/storybook/issues/22431
jackspeak: '2.1.1',
});

await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
await withLocalRegistry({
packageManager,
action: async () => {
await packageManager.addPackageResolutions({
...storybookVersions,
// Yarn1 Issue: https://github.com/storybookjs/storybook/issues/22431
jackspeak: '2.1.1',
});

await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
},
cwd: tmpDir,
env,
debug,
});
} else {
await sbInit(tmpDir, env, [...flags, '--package-manager=yarn1'], debug);
Expand Down Expand Up @@ -159,7 +181,7 @@ const runGenerators = async (
const baseDir = join(REPROS_DIRECTORY, dirName);
const beforeDir = join(baseDir, BEFORE_DIR_NAME);
try {
let flags: string[] = [];
let flags: string[] = ['--no-dev'];
if (expected.renderer === '@storybook/html') flags = ['--type html'];
else if (expected.renderer === '@storybook/server') flags = ['--type server'];

Expand Down