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

Switch to a new progress event for build status #55

Closed
Closed
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
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
"dependencies": {
"@storybook/csf-tools": "7.4.0-alpha.2",
"@storybook/design-system": "^7.15.15",
"chromatic": "6.24.0",
"chromatic": "6.25.0-canary.0",
"date-fns": "^2.30.0",
"pluralize": "^8.0.0",
"urql": "^4.0.3",
Expand Down
16 changes: 10 additions & 6 deletions src/Panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ import React, { useCallback, useState } from "react";

import {
ADDON_ID,
BUILD_ANNOUNCED,
BUILD_STARTED,
BUILD_PROGRESS,
BuildProgressPayload,
DEV_BUILD_ID_KEY,
GIT_INFO,
GitInfoPayload,
Expand Down Expand Up @@ -60,10 +60,14 @@ export const Panel = ({ active, api }: PanelProps) => {
const emit = useChannel(
{
[START_BUILD]: () => setIsStarting(true),
[BUILD_STARTED]: () => setIsStarting(false),
[BUILD_ANNOUNCED]: (buildId: string) => {
setLastBuildId(buildId);
localStorage.setItem(DEV_BUILD_ID_KEY, buildId);
[BUILD_PROGRESS]: ({ step, id }: BuildProgressPayload) => {
if (step === "build") {
setLastBuildId(id);
localStorage.setItem(DEV_BUILD_ID_KEY, id);
}
if (step === "snapshot" || step === "complete") {
setIsStarting(false);
}
},
[GIT_INFO]: (info: GitInfoPayload) => {
setGitInfo(info);
Expand Down
8 changes: 6 additions & 2 deletions src/Tool.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,19 @@ import { useChannel } from "@storybook/manager-api";
import React, { useState } from "react";

import { ProgressIcon } from "./components/icons/ProgressIcon";
import { BUILD_STARTED, START_BUILD, TOOL_ID } from "./constants";
import { BUILD_PROGRESS, BuildProgressPayload, START_BUILD, TOOL_ID } from "./constants";

export const Tool = () => {
const [isStarting, setIsStarting] = useState(false);

const emit = useChannel(
{
[START_BUILD]: () => setIsStarting(true),
[BUILD_STARTED]: () => setIsStarting(false),
[BUILD_PROGRESS]: ({ step }: BuildProgressPayload) => {
if (step === "snapshot" || step === "complete") {
setIsStarting(false);
}
},
},
[]
);
Expand Down
15 changes: 13 additions & 2 deletions src/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,20 @@ export const DEV_BUILD_ID_KEY = `${ADDON_ID}/dev-build-id`;

export const GIT_INFO = `${ADDON_ID}/gitInfo`;
export type GitInfoPayload = Omit<GitInfo, "committedAt" | "committerEmail" | "committerName">;

export const START_BUILD = `${ADDON_ID}/startBuild`;
export const BUILD_ANNOUNCED = `${ADDON_ID}/buildAnnounced`;
export const BUILD_STARTED = `${ADDON_ID}/buildStarted`;
export const BUILD_PROGRESS = `${ADDON_ID}/buildProgress`;
export type BuildProgressPayload = {
// Possibly this should be a type exported by the CLI -- these correspond to tasks
/** The step of the build process we have reached */
step: "initialize" | "build" | "upload" | "verify" | "snapshot" | "complete";
/** The id of the build, available after the initialize step */
id?: string;
/** progress pertains to the current step, and may not be set */
progress?: number;
/** total pertains to the current step, and may not be set */
total?: number;
};

export const UPDATE_PROJECT = `${ADDON_ID}/updateProject`;
export type UpdateProjectPayload = {
Expand Down
50 changes: 36 additions & 14 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import { readConfig, writeConfig } from "@storybook/csf-tools";
import { getGitInfo, GitInfo, run } from "chromatic/node";

import {
BUILD_ANNOUNCED,
BUILD_STARTED,
BUILD_PROGRESS,
BuildProgressPayload,
CHROMATIC_ADDON_NAME,
CHROMATIC_BASE_URL,
GIT_INFO,
Expand Down Expand Up @@ -55,8 +55,7 @@ async function serverChannel(
) {
let projectToken = initialProjectToken;
channel.on(START_BUILD, async () => {
let announced = false;
let started = false;
let step: BuildProgressPayload["step"] = "initialize";
await run({
// Currently we have to have these flags.
// We should move the checks to after flags have been parsed into options.
Expand All @@ -70,19 +69,42 @@ async function serverChannel(
// Builds initiated from the addon are always considered local
isLocalBuild: true,
onTaskComplete(ctx) {
console.log(`Completed task '${ctx.title}'`);
if (!announced && ctx.announcedBuild) {
console.debug("emitting", BUILD_ANNOUNCED, ctx.announcedBuild.id);
channel.emit(BUILD_ANNOUNCED, ctx.announcedBuild.id);
announced = true;
let newStep: BuildProgressPayload["step"];
if (step === "initialize" && ctx.announcedBuild) {
newStep = "build";
}
if (announced && !started && ctx.build) {
console.debug("emitting", BUILD_STARTED, ctx.build.status);
channel.emit(BUILD_STARTED, ctx.build.status);
started = true;
if (step === "upload" && ctx.isolatorUrl) {
newStep = "verify";
}
if (["build", "upload"].includes(step) && ctx.build) {
newStep = "snapshot";
}
if (ctx.build?.status !== "IN_PROGRESS") {
newStep = "complete";
}
Comment on lines +72 to +84
Copy link
Member Author

Choose a reason for hiding this comment

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

Not quite sure about this logic, @ghengeveld do you have a better idea?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not digging the let newStep. I'd rather have a function that returns the new step.

As you know I don't like let at all. I can sort of let it slide when it's used for memory storage over time (such as step above), but not when it's used with a series of if statements to (synchronously) determine its final value.

Copy link
Member

Choose a reason for hiding this comment

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

I think what I'd like to see is a map (or other iteration) of steps, each specifying how they'd react to onTaskComplete and/or onTaskProgress events. Something like:

const steps = {
  _current: steps.announce,

  go(ctx, step) {
    channel.emit(BUILD_PROGRESS, { step, buildId: ctx.announcedBuild?.id });
    steps._current = steps[step];
  },

  announce: {
    onTaskComplete(ctx) {
      if (ctx.announcedBuild) steps.go(ctx, "build");
    }
  },
  build: {
    onTaskProgress(ctx, data) {
      if (data.unit === "bytes") {
        steps.go(ctx, "publish");
        steps.publish.onTaskProgress(ctx, data);
      };
    }
  },
  publish: {
    onTaskComplete(ctx) {
      if (ctx.isolatorUrl) steps.go(ctx, "verify");
    },
    onTaskProgress({ announcedBuild }, { progress, total, unit }) {
      channel.emit(BUILD_PROGRESS, { ... });
    }
  },
  verify: {
    onTaskComplete({ build }) {
      if (build) steps.go(ctx, "test");
    },
  },
  test: {
    onTaskComplete({ build }) {
      if (build.completedAt) steps.go(ctx, "complete");
    },
    onTaskProgress({ announcedBuild }, { progress, total, unit }) {
      channel.emit(BUILD_PROGRESS, { ... });
    }
  },
  complete: {}
}

Note I also renamed initialize to announce and snapshot to test.


if (newStep !== step) {
step = newStep;
channel.emit(BUILD_PROGRESS, {
step,
id: ctx.announcedBuild?.id,
} satisfies BuildProgressPayload);
}
},
onTaskProgress(ctx, { progress, total, unit }) {
if (unit === "bytes") {
step = "upload";
} else {
step = "snapshot";
}

channel.emit(BUILD_PROGRESS, {
step,
id: ctx.announcedBuild.id,
progress,
total,
} satisfies BuildProgressPayload);
},
// as any due to CLI mistyping: https://github.com/chromaui/chromatic-cli/pull/800
},
});
});
Expand Down
20 changes: 20 additions & 0 deletions src/screens/VisualTests/BuildStatus.stories.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import type { Meta, StoryObj } from "@storybook/react";

import { withFigmaDesign } from "../../utils/withFigmaDesign";
import { BuildStatus } from "./BuildStatus";

const meta = {
component: BuildStatus,
} satisfies Meta<typeof BuildStatus>;

export default meta;
type Story = StoryObj<typeof meta>;

export const Default: Story = {
args: {
// build: passedBuild,
},
parameters: withFigmaDesign(
"https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=508-525764&t=18c1zI1SMe76dWYk-4"
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to link to the "Render settings" screen?

),
};
5 changes: 5 additions & 0 deletions src/screens/VisualTests/BuildStatus.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
import React from "react";

export function BuildStatus({}) {
return <div>Build Status</div>;
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this intentionally unfinished?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, there is a second PR that actually implements the UI: #56

8 changes: 4 additions & 4 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -5453,10 +5453,10 @@ chownr@^2.0.0:
resolved "https://registry.yarnpkg.com/chownr/-/chownr-2.0.0.tgz#15bfbe53d2eab4cf70f18a8cd68ebe5b3cb1dece"
integrity sha512-bIomtDF5KGpdogkLd9VspvFzk9KfpyyGlS8YFVZl7TGPBHL5snIOnxeshwVgPteQ9b4Eydl+pVbIyE1DcvCWgQ==

chromatic@6.24.0:
version "6.24.0"
resolved "https://registry.yarnpkg.com/chromatic/-/chromatic-6.24.0.tgz#007561facf1c3d444195cf640195dbb19c2cc4e1"
integrity sha512-i09yqXa5P1qsNntohW8SN3MYWitdz8j6FazOGrN7v2nEvPauP50vTKFYKxBXq68kp904ede/06D4TZdkfp55sw==
chromatic@6.25.0-canary.0:
version "6.25.0-canary.0"
resolved "https://registry.yarnpkg.com/chromatic/-/chromatic-6.25.0-canary.0.tgz#1121c5afcf335a2b5ee1fbe5c709b0f42dbbd1d3"
integrity sha512-oiytax7ozb/b4bFLTRuDQ60uUxpmT7W/O4yjm49YhYfpXFbKsDMDyyLjJZnE3ntMJxyhF5uSehuZFnS0CXaemw==

ci-info@^3.2.0:
version "3.8.0"
Expand Down
Loading