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

Handle redirects when using --storybook-url #596

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
12 changes: 6 additions & 6 deletions bin-src/lib/getOptions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ describe('getOptions', () => {
it('picks up default start script', async () => {
expect(getOptions(getContext(['-s']))).toMatchObject({
scriptName: 'storybook',
url: 'http://localhost:1337/iframe.html',
url: 'http://localhost:1337',
noStart: false,
});
});
Expand All @@ -95,7 +95,7 @@ describe('getOptions', () => {
it('allows you to specify alternate script, still picks up port', async () => {
expect(getOptions(getContext(['--script-name', 'otherStorybook']))).toMatchObject({
scriptName: 'otherStorybook',
url: 'http://localhost:7070/iframe.html',
url: 'http://localhost:7070',
noStart: false,
});
});
Expand All @@ -105,7 +105,7 @@ describe('getOptions', () => {
getOptions(getContext(['--script-name', 'notStorybook', '--storybook-port', '6060']))
).toMatchObject({
scriptName: 'notStorybook',
url: 'http://localhost:6060/iframe.html',
url: 'http://localhost:6060',
});
});

Expand All @@ -120,7 +120,7 @@ describe('getOptions', () => {
getOptions(getContext(['--exec', 'storybook-command', '--storybook-port', '6060']))
).toMatchObject({
exec: 'storybook-command',
url: 'http://localhost:6060/iframe.html',
url: 'http://localhost:6060',
});
});

Expand Down Expand Up @@ -169,15 +169,15 @@ describe('getOptions', () => {
it('allows you to set a URL without path', async () => {
expect(getOptions(getContext(['--storybook-url', 'https://google.com']))).toMatchObject({
noStart: true,
url: 'https://google.com/iframe.html',
url: 'https://google.com',
createTunnel: false,
});
});

it('allows you to set a URL with a path', async () => {
expect(getOptions(getContext(['--storybook-url', 'https://google.com/foo']))).toMatchObject({
noStart: true,
url: 'https://google.com/foo/iframe.html',
url: 'https://google.com/foo',
createTunnel: false,
});
});
Expand Down
11 changes: 1 addition & 10 deletions bin-src/lib/getOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -221,20 +221,11 @@ export default function getOptions({ argv, env, flags, log, packageJson }: Conte
storybookUrl = `${options.https ? 'https' : 'http'}://localhost:${port}`;
}

const parsedUrl = new URL(storybookUrl);
const suffix = 'iframe.html';
if (!parsedUrl.pathname.endsWith(suffix)) {
if (!parsedUrl.pathname.endsWith('/')) {
parsedUrl.pathname += '/';
}
parsedUrl.pathname += suffix;
}

return {
...options,
noStart,
useTunnel: true,
url: parsedUrl.href,
url: storybookUrl,
scriptName,
};
}
22 changes: 13 additions & 9 deletions bin-src/lib/startStorybook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,22 @@ import { spawn } from 'cross-spawn';
import path from 'path';
import { Context } from '../types';

export async function checkResponse(ctx: Context, url: string) {
export async function resolveIsolatorUrl(ctx: Context, storybookUrl: string) {
try {
// Allow invalid certificates, because we're running against localhost
await ctx.http.fetch(url, {}, { proxy: { rejectUnauthorized: false } });
return true;
// Allow invalid certificates, because we might be running against localhost.
const options = { proxy: { rejectUnauthorized: false } };
const { url: resolvedUrl } = await ctx.http.fetch(storybookUrl, {}, options);
const isolatorUrl = resolvedUrl.replace(/index\.html$/, '').replace(/\/?$/, '/iframe.html');
ghengeveld marked this conversation as resolved.
Show resolved Hide resolved
await ctx.http.fetch(isolatorUrl, {}, options);
return isolatorUrl;
} catch (e) {
return false;
}
}

async function waitForResponse(ctx: Context, child: ReturnType<typeof spawn>, url: string) {
const timeoutAt = Date.now() + ctx.env.CHROMATIC_TIMEOUT;
return new Promise<void>((resolve, reject) => {
return new Promise<string>((resolve, reject) => {
let resolved = false;
async function check() {
if (Date.now() > timeoutAt) {
Expand All @@ -28,9 +31,10 @@ async function waitForResponse(ctx: Context, child: ReturnType<typeof spawn>, ur
return;
}

if (await checkResponse(ctx, url)) {
const isolatorUrl = await resolveIsolatorUrl(ctx, url);
if (isolatorUrl) {
resolved = true;
resolve();
resolve(isolatorUrl);
return;
}
setTimeout(check, ctx.env.CHROMATIC_POLL_INTERVAL);
Expand Down Expand Up @@ -61,7 +65,7 @@ export default async function startApp(
) {
let child: ReturnType<typeof spawn>;
if (ctx.options.scriptName) {
if (await checkResponse(ctx, ctx.options.url)) {
if (await resolveIsolatorUrl(ctx, ctx.options.url)) {
// We assume the process that is already running on the url is indeed our Storybook
return null;
}
Expand All @@ -84,7 +88,7 @@ export default async function startApp(
}

if (ctx.options.url) {
await waitForResponse(ctx, child, ctx.options.url);
ctx.isolatorUrl = await waitForResponse(ctx, child, ctx.options.url);
}

return child;
Expand Down
49 changes: 34 additions & 15 deletions bin-src/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,8 @@ const getCommit = <jest.MockedFunction<typeof git.getCommit>>git.getCommit;
jest.mock('./lib/startStorybook');

const startApp = <jest.MockedFunction<typeof startStorybook.default>>startStorybook.default;
const checkResponse = <jest.MockedFunction<typeof startStorybook.checkResponse>>(
startStorybook.checkResponse
const resolveIsolatorUrl = <jest.MockedFunction<typeof startStorybook.resolveIsolatorUrl>>(
startStorybook.resolveIsolatorUrl
);

jest.mock('./lib/getStorybookInfo', () => () => ({
Expand Down Expand Up @@ -416,12 +416,15 @@ it('passes autoAcceptChanges to the index based on branch', async () => {

describe('tunneled build', () => {
beforeEach(() => {
startApp.mockReset().mockResolvedValue({
on: jest.fn(),
stderr: { on: jest.fn(), resume: jest.fn() },
stdout: { on: jest.fn(), resume: jest.fn() },
} as any);
checkResponse.mockReset();
resolveIsolatorUrl.mockReset().mockResolvedValue(false);
startApp.mockReset().mockImplementation((ctx) => {
ctx.isolatorUrl = ctx.options.url;
return {
on: jest.fn(),
stderr: { on: jest.fn(), resume: jest.fn() },
stdout: { on: jest.fn(), resume: jest.fn() },
} as any;
});
openTunnel.mockReset().mockResolvedValue({
url: 'http://tunnel.com/?clientId=foo',
cachedUrl: 'http://cached.tunnel.com?foo=bar#hash',
Expand All @@ -431,6 +434,7 @@ describe('tunneled build', () => {
});

it('properly deals with updating the isolatorUrl/cachedUrl in complex situations', async () => {
resolveIsolatorUrl.mockResolvedValue('http://localhost:1337/iframe.html');
const ctx = getContext(['--project-token=asdf1234', '--script-name=storybook']);
await runBuild(ctx);

Expand All @@ -449,7 +453,7 @@ describe('tunneled build', () => {
expect.objectContaining({
options: expect.objectContaining({
scriptName: 'storybook',
url: 'http://localhost:1337/iframe.html',
url: 'http://localhost:1337',
}),
}),
expect.objectContaining({
Expand All @@ -469,7 +473,7 @@ describe('tunneled build', () => {
expect.objectContaining({
options: expect.objectContaining({
exec: './run.sh',
url: 'http://localhost:9001/iframe.html',
url: 'http://localhost:9001',
}),
}),
expect.objectContaining({})
Expand All @@ -481,7 +485,7 @@ describe('tunneled build', () => {
});

it('skips start when already running', async () => {
checkResponse.mockResolvedValue(true);
resolveIsolatorUrl.mockResolvedValue('http://localhost:1337/iframe.html');
const ctx = getContext(['--project-token=asdf1234', '--script-name=storybook']);
await runBuild(ctx);
expect(startApp).not.toHaveBeenCalled();
Expand All @@ -492,7 +496,7 @@ describe('tunneled build', () => {
});

it('fails when trying to use --do-not-start while not running', async () => {
checkResponse.mockResolvedValueOnce(false);
resolveIsolatorUrl.mockResolvedValueOnce(false);
const ctx = getContext([
'--project-token=asdf1234',
'--script-name=storybook',
Expand All @@ -504,7 +508,7 @@ describe('tunneled build', () => {
});

it('skips tunnel when using --storybook-url', async () => {
checkResponse.mockResolvedValue(true);
resolveIsolatorUrl.mockResolvedValue('http://localhost:1337/iframe.html');
const ctx = getContext([
'--project-token=asdf1234',
'--storybook-url=http://localhost:1337/iframe.html?foo=bar#hash',
Expand All @@ -514,15 +518,30 @@ describe('tunneled build', () => {
expect(ctx.closeTunnel).toBeUndefined();
expect(openTunnel).not.toHaveBeenCalled();
expect(publishedBuild).toMatchObject({
isolatorUrl: 'http://localhost:1337/iframe.html?foo=bar#hash',
isolatorUrl: 'http://localhost:1337/iframe.html',
});
});

it('supports redirects in --storybook-url', async () => {
resolveIsolatorUrl.mockResolvedValue('http://localhost:1337/some/other/path/iframe.html');
const ctx = getContext([
'--project-token=asdf1234',
'--storybook-url=http://localhost:1337/iframe.html?foo=bar#hash',
]);
await runBuild(ctx);
expect(publishedBuild).toMatchObject({
isolatorUrl: 'http://localhost:1337/some/other/path/iframe.html',
});
});

it('stops the running Storybook if something goes wrong', async () => {
openTunnel.mockImplementation(() => {
throw new Error('tunnel error');
});
startApp.mockResolvedValueOnce({ pid: 12345 } as any);
startApp.mockReset().mockImplementation((ctx) => {
ctx.isolatorUrl = ctx.options.url;
return { pid: 12345 } as any;
});
const ctx = getContext(['--project-token=asdf1234', '--script-name=storybook']);
await runBuild(ctx);
expect(ctx.exitCode).toBe(255);
Expand Down
1 change: 0 additions & 1 deletion bin-src/tasks/start.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ describe('startStorybook', () => {
},
} as any;
await startStorybook(ctx);
expect(ctx.isolatorUrl).toBe(ctx.options.url);
expect(startApp).toHaveBeenCalledWith(ctx, {
args: undefined,
options: { stdio: 'pipe' },
Expand Down
12 changes: 5 additions & 7 deletions bin-src/tasks/start.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import semver from 'semver';
import treeKill from 'tree-kill';

import startApp, { checkResponse } from '../lib/startStorybook';
import startApp, { resolveIsolatorUrl } from '../lib/startStorybook';
import { createTask, transitionTo } from '../lib/tasks';
import { Context } from '../types';
import { initial, pending, skipFailed, skipped, success } from '../ui/tasks/start';

export const startStorybook = async (ctx: Context) => {
const { scriptName, url } = ctx.options;

const child = await startApp(ctx, {
args: scriptName &&
args: ctx.options.scriptName &&
ctx.storybook.version &&
semver.gte(ctx.storybook.version, ctx.env.STORYBOOK_CLI_FLAGS_BY_VERSION['--ci']) && [
'--',
Expand All @@ -19,7 +17,6 @@ export const startStorybook = async (ctx: Context) => {
options: { stdio: 'pipe' },
});

ctx.isolatorUrl = url;
ctx.stopApp = () =>
child &&
new Promise((resolve, reject) =>
Expand All @@ -31,8 +28,9 @@ export default createTask({
title: initial.title,
skip: async (ctx: Context) => {
if (ctx.skip) return true;
if (await checkResponse(ctx, ctx.options.url)) {
ctx.isolatorUrl = ctx.options.url;
const isolatorUrl = await resolveIsolatorUrl(ctx, ctx.options.url);
if (isolatorUrl) {
ctx.isolatorUrl = isolatorUrl;
return skipped(ctx).output;
}
if (ctx.options.noStart) {
Expand Down