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

Turn on strict types (!) #110

Merged
merged 8 commits into from
Sep 18, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
8 changes: 8 additions & 0 deletions src/Panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ export const Panel = ({ active, api }: PanelProps) => {
);

if (projectUpdatingFailed) {
// These should always be set when we get this error
if (!projectToken || !configFile) {
throw new Error(`Missing projectToken/config file after configuration failure`);
}

return (
<Sections hidden={!active}>
<LinkingProjectFailed
Expand All @@ -86,6 +91,9 @@ export const Panel = ({ active, api }: PanelProps) => {
}

if (projectIdUpdated) {
// This should always be set when we succeed
if (!configFile) throw new Error(`Missing config file after configuration success`);

return (
<Provider key={PANEL_ID} value={client}>
<Sections hidden={!active}>
Expand Down
3 changes: 3 additions & 0 deletions src/SidebarTop.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export const SidebarTop = ({ api }: SidebarTopProps) => {
name: "passed",
color: color.positive,
},
// @ts-expect-error SB needs a proper API for no link
link: undefined,
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
});
setTimeout(() => clearNotification(`${ADDON_ID}/build-initialize`), 10_000);
Expand All @@ -62,6 +63,7 @@ export const SidebarTop = ({ api }: SidebarTopProps) => {
name: "passed",
color: color.positive,
},
// @ts-expect-error SB needs a proper API for no link
link: undefined,
});
setTimeout(() => clearNotification(`${ADDON_ID}/build-complete`), 10_000);
Expand All @@ -78,6 +80,7 @@ export const SidebarTop = ({ api }: SidebarTopProps) => {
name: "failed",
color: color.negative,
},
// @ts-expect-error SB needs a proper API for no link
link: undefined,
});
}
Expand Down
2 changes: 1 addition & 1 deletion src/buildSteps.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export const BUILD_STEP_CONFIG: Record<
renderName: () => `Publish your Storybook`,
renderProgress: ({ stepProgress }) => {
const { numerator, denominator } = stepProgress.upload;
if (!denominator) return `Uploading files`;
if (!denominator || !numerator) return `Uploading files`;
const { value: total, exponent } = filesize(denominator, {
output: "object",
round: 1,
Expand Down
27 changes: 15 additions & 12 deletions src/components/BrowserSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ type BrowserData = Pick<BrowserInfo, "id" | "key" | "name">;
interface BrowserSelectorProps {
isAccepted: boolean;
selectedBrowser: BrowserData;
browserResults: { browser: BrowserData; result: ComparisonResult }[];
browserResults: { browser: BrowserData; result?: ComparisonResult }[];
onSelectBrowser: (browser: BrowserData) => void;
}

Expand All @@ -53,17 +53,20 @@ export const BrowserSelector = ({
type Link = ComponentProps<typeof TooltipMenu>["links"][0];

const links: Link[] =
browserResults.length > 1 &&
browserResults.map(
({ browser, result }): Link => ({
active: selectedBrowser === browser,
id: browser.id,
onClick: () => onSelectBrowser(browser),
right: !isAccepted && result !== ComparisonResult.Equal && <StatusDot status={result} />,
icon: browserIcons[browser.key],
title: browser.name,
})
);
browserResults.length > 1
? browserResults.map(
({ browser, result }): Link => ({
active: selectedBrowser === browser,
id: browser.id,
onClick: () => onSelectBrowser(browser),
right: !isAccepted && result !== ComparisonResult.Equal && (
<StatusDot status={result} />
),
icon: browserIcons[browser.key],
title: browser.name,
})
)
: [];

return (
<WithTooltip
Expand Down
2 changes: 1 addition & 1 deletion src/components/ModeSelector.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ interface ModeSelectorProps {
isAccepted: boolean;
selectedMode: ModeData;
onSelectMode: (viewport: ModeData) => void;
modeResults: { viewport: ModeData; result: ComparisonResult }[];
modeResults: { viewport: ModeData; result?: ComparisonResult }[];
}

export const ModeSelector = ({
Expand Down
2 changes: 1 addition & 1 deletion src/components/SnapshotImage.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export const TallerConstrained: Story = {

export const CaptureError: Story = {
args: {
captureImage: null,
captureImage: undefined,
comparisonResult: ComparisonResult.CaptureError,
},
};
6 changes: 3 additions & 3 deletions src/components/SnapshotImage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,10 @@ export const Container = styled.div<{ href?: string; target?: string }>(
);

interface SnapshotImageProps {
componentName: Test["story"]["component"]["name"];
storyName: Test["story"]["name"];
componentName?: NonNullable<NonNullable<Test["story"]>["component"]>["name"];
tmeasday marked this conversation as resolved.
Show resolved Hide resolved
storyName?: NonNullable<Test["story"]>["name"];
testUrl: Test["webUrl"];
comparisonResult: ComparisonResult;
comparisonResult?: ComparisonResult;
captureImage?: Pick<CaptureImage, "imageUrl" | "imageWidth">;
diffImage?: Pick<CaptureOverlayImage, "imageUrl" | "imageWidth">;
focusImage?: Pick<CaptureOverlayImage, "imageUrl" | "imageWidth">;
Expand Down
36 changes: 19 additions & 17 deletions src/components/StatusDot.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,23 +13,25 @@ const Dot = styled.div<StatusDotProps & { overlay?: boolean }>(
width: 6,
height: 6,
borderRadius: "50%",
background: {
[TestStatus.InProgress]: "transparent",
[TestStatus.Passed]: theme.color.positive,
[TestStatus.Pending]: theme.color.gold,
[TestStatus.Accepted]: theme.color.positive,
[TestStatus.Denied]: theme.color.positive,
[TestStatus.Broken]: theme.color.negative,
[TestStatus.Failed]: theme.color.negative,
[ComparisonResult.Equal]: theme.color.positive,
[ComparisonResult.Fixed]: theme.color.positive,
[ComparisonResult.Added]: theme.color.gold,
[ComparisonResult.Changed]: theme.color.gold,
[ComparisonResult.Removed]: theme.color.gold,
[ComparisonResult.CaptureError]: theme.color.negative,
[ComparisonResult.SystemError]: theme.color.negative,
notification: theme.color.secondary,
}[status],
background:
status &&
{
[TestStatus.InProgress]: "transparent",
[TestStatus.Passed]: theme.color.positive,
[TestStatus.Pending]: theme.color.gold,
[TestStatus.Accepted]: theme.color.positive,
[TestStatus.Denied]: theme.color.positive,
[TestStatus.Broken]: theme.color.negative,
[TestStatus.Failed]: theme.color.negative,
[ComparisonResult.Equal]: theme.color.positive,
[ComparisonResult.Fixed]: theme.color.positive,
[ComparisonResult.Added]: theme.color.gold,
[ComparisonResult.Changed]: theme.color.gold,
[ComparisonResult.Removed]: theme.color.gold,
[ComparisonResult.CaptureError]: theme.color.negative,
[ComparisonResult.SystemError]: theme.color.negative,
notification: theme.color.secondary,
}[status],
}),
({ overlay, theme }) =>
overlay &&
Expand Down
4 changes: 2 additions & 2 deletions src/components/SuffixInput.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ const SuffixOverlay = ({
placeholder,
suffix,
}: {
value: string;
placeholder: string;
value?: string;
placeholder?: string;
suffix: ReactNode;
}) => (
<SuffixWrapper>
Expand Down
2 changes: 1 addition & 1 deletion src/components/TooltipMenu.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const TooltipMenu = ({ children, links, note, ...props }: TooltipMenuProp
...link,
onClick: (...args) => {
onHide();
return link.onClick(...args);
return link.onClick?.(...args);
},
}))}
/>
Expand Down
4 changes: 2 additions & 2 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const observeGitInfo = async (
callback: (info: GitInfo, prevInfo: GitInfo) => void
) => {
let prev: GitInfo;
let timer: NodeJS.Timeout | null = null;
let timer: NodeJS.Timeout | undefined;
const act = async () => {
const gitInfo = await getGitInfo();
if (Object.entries(gitInfo).some(([key, value]) => prev?.[key as keyof GitInfo] !== value)) {
Expand Down Expand Up @@ -93,7 +93,7 @@ async function serverChannel(
const localBuildProgress = useAddonState<LocalBuildProgress>(channel, LOCAL_BUILD_PROGRESS);

channel.on(START_BUILD, async () => {
const { projectToken } = projectInfoState.value;
const { projectToken } = projectInfoState.value || {};
await runChromaticBuild(localBuildProgress, { projectToken });
});

Expand Down
2 changes: 1 addition & 1 deletion src/manager.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ addons.register(ADDON_ID, (api) => {
type: types.PANEL,
title: "Visual Tests",
match: ({ viewMode }) => viewMode === "story",
render: ({ active }) => <Panel active={active} api={api} />,
render: ({ active }) => <Panel active={!!active} api={api} />,
});

addons.add(SIDEBAR_TOP_ID, {
Expand Down
38 changes: 21 additions & 17 deletions src/runChromaticBuild.test.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,17 @@
import { INITIAL_BUILD_PAYLOAD } from "./buildSteps";
import { onCompleteOrError, onStartOrProgress, runChromaticBuild } from "./runChromaticBuild";
import { LocalBuildProgress } from "./types";

jest.mock("chromatic/node", () => ({ run: jest.fn() }));

const store = {
state: undefined as LocalBuildProgress | undefined,
get value() {
if (!this.state) throw new Error("need to call set first");

return this.state;
},
set value(newValue) {
set value(newValue: LocalBuildProgress) {
this.state = newValue;
},
on() {},
Expand Down Expand Up @@ -39,28 +43,28 @@ describe("onStartOrProgress", () => {
currentStep: "initialize",
});

onStartOrProgress(store, null)({ task: "build" } as any);
onStartOrProgress(store)({ task: "build" } as any);
expect(store.value).toMatchObject({
buildProgressPercentage: expect.closeTo(3, 0),
currentStep: "build",
stepProgress: { build: { startedAt: expect.any(Number) } },
});

onStartOrProgress(store, null)({ task: "upload" } as any);
onStartOrProgress(store)({ task: "upload" } as any);
expect(store.value).toMatchObject({
buildProgressPercentage: expect.closeTo(24, 0),
currentStep: "upload",
stepProgress: { upload: { startedAt: expect.any(Number) } },
});

onStartOrProgress(store, null)({ task: "verify" } as any);
onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value).toMatchObject({
buildProgressPercentage: expect.closeTo(48, 0),
currentStep: "verify",
stepProgress: { verify: { startedAt: expect.any(Number) } },
});

onStartOrProgress(store, null)({ task: "snapshot" } as any);
onStartOrProgress(store)({ task: "snapshot" } as any);
expect(store.value).toMatchObject({
buildProgressPercentage: expect.closeTo(55, 0),
currentStep: "snapshot",
Expand All @@ -69,36 +73,36 @@ describe("onStartOrProgress", () => {
});

it("updates progress with each invocation", () => {
onStartOrProgress(store, null)({ task: "verify" } as any);
onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(48, 0);

onStartOrProgress(store, null)({ task: "verify" } as any);
onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(50, 0);

onStartOrProgress(store, null)({ task: "verify" } as any);
onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(52, 0);
});

it("can never exceed progress for a step beyond the next step", () => {
for (let n = 10; n; n -= 1) onStartOrProgress(store, null)({ task: "verify" } as any);
for (let n = 10; n; n -= 1) onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(55, 0);

for (let n = 10; n; n -= 1) onStartOrProgress(store, null)({ task: "verify" } as any);
for (let n = 10; n; n -= 1) onStartOrProgress(store)({ task: "verify" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(55, 0);

onStartOrProgress(store, null)({ task: "snapshot" } as any);
onStartOrProgress(store)({ task: "snapshot" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(55, 0);
});

it('updates build progress based on "progress" and "total" values', () => {
onStartOrProgress(store, null)({ task: "snapshot" } as any);
onStartOrProgress(store)({ task: "snapshot" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(55, 0);

onStartOrProgress(store, null)({ task: "snapshot" } as any, { progress: 1, total: 2 });
onStartOrProgress(store)({ task: "snapshot" } as any, { progress: 1, total: 2 });
expect(store.value.stepProgress.snapshot).toMatchObject({ numerator: 1, denominator: 2 });
expect(store.value.buildProgressPercentage).toBeCloseTo(77, 0);

onStartOrProgress(store, null)({ task: "snapshot" } as any, { progress: 2, total: 2 });
onStartOrProgress(store)({ task: "snapshot" } as any, { progress: 2, total: 2 });
expect(store.value.stepProgress.snapshot).toMatchObject({ numerator: 2, denominator: 2 });
expect(store.value.buildProgressPercentage).toBeCloseTo(100, 0);
});
Expand All @@ -111,7 +115,7 @@ describe("onCompleteOrError", () => {

it("sets build progress to 100% on completion of final step", () => {
const build = { changeCount: 1, errorCount: 2 };
onCompleteOrError(store, null)({ task: "snapshot", build } as any);
onCompleteOrError(store)({ task: "snapshot", build } as any);
expect(store.value).toMatchObject({
buildProgressPercentage: 100,
currentStep: "complete",
Expand All @@ -122,11 +126,11 @@ describe("onCompleteOrError", () => {
});

it("does not set build progress to 100% on error", () => {
onStartOrProgress(store, null)({ task: "snapshot" } as any);
onStartOrProgress(store)({ task: "snapshot" } as any);
expect(store.value.buildProgressPercentage).toBeCloseTo(55, 0);

const error = { formattedError: "Oops!", originalError: new Error("Oops!") };
onCompleteOrError(store, null)({ task: "snapshot" } as any, error);
onCompleteOrError(store)({ task: "snapshot" } as any, error);
expect(store.value).toMatchObject({
buildProgressPercentage: expect.closeTo(55, 0),
currentStep: "error",
Expand Down
21 changes: 18 additions & 3 deletions src/runChromaticBuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,13 +45,18 @@ const getBuildStepData = (
export const onStartOrProgress =
(
localBuildProgress: ReturnType<typeof useAddonState<LocalBuildProgress>>,
timeout: ReturnType<typeof setTimeout>
timeout?: ReturnType<typeof setTimeout>
) =>
({ ...ctx }: Context, { progress, total }: { progress?: number; total?: number } = {}) => {
clearTimeout(timeout);

if (!isKnownStep(ctx.task)) return;

// We should set this right before starting so it should never be unset during a build.
if (!localBuildProgress.value) {
throw new Error("Unexpected missing value for localBuildProgress");
}

const { buildProgressPercentage, stepProgress, previousBuildProgress } =
localBuildProgress.value;

Expand All @@ -74,6 +79,11 @@ export const onStartOrProgress =
(ESTIMATED_PROGRESS_INTERVAL / estimateDuration) * stepPercentage;

timeout = setTimeout(() => {
// We should set this right before starting so it should never be unset during a build.
if (!localBuildProgress.value) {
throw new Error("Unexpected missing value for localBuildProgress");
}

// Intentionally reference the present value here (after timeout)
const { currentStep } = localBuildProgress.value;
// Only update if we haven't moved on to a later step
Expand All @@ -100,11 +110,16 @@ export const onStartOrProgress =
export const onCompleteOrError =
(
localBuildProgress: ReturnType<typeof useAddonState<LocalBuildProgress>>,
timeout: ReturnType<typeof setTimeout>
timeout?: ReturnType<typeof setTimeout>
) =>
(ctx: Context, error?: { formattedError: string; originalError: Error | Error[] }) => {
clearTimeout(timeout);

// We should set this right before starting so it should never be unset during a build.
if (!localBuildProgress.value) {
throw new Error("Unexpected missing value for localBuildProgress");
}

const { buildProgressPercentage, stepProgress } = localBuildProgress.value;

if (isKnownStep(ctx.task)) {
Expand Down Expand Up @@ -149,7 +164,7 @@ export const runChromaticBuild = async (
localBuildProgress.value = INITIAL_BUILD_PAYLOAD;

// Timeout is defined here so it's shared between all handlers
let timeout: ReturnType<typeof setTimeout>;
let timeout: ReturnType<typeof setTimeout> | undefined;

await run({
// Currently we have to have these flags.
Expand Down
Loading
Loading