Switch to a new progress event for build status #55
Conversation
| 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"; | ||
| } |
There was a problem hiding this comment.
Not quite sure about this logic, @ghengeveld do you have a better idea?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| export function BuildStatus({}) { | ||
| return <div>Build Status</div>; | ||
| } |
There was a problem hiding this comment.
Is this intentionally unfinished?
There was a problem hiding this comment.
Yes, there is a second PR that actually implements the UI: #56
| // build: passedBuild, | ||
| }, | ||
| parameters: withFigmaDesign( | ||
| "https://www.figma.com/file/GFEbCgCVDtbZhngULbw2gP/Visual-testing-in-Storybook?type=design&node-id=508-525764&t=18c1zI1SMe76dWYk-4" |
There was a problem hiding this comment.
Is this supposed to link to the "Render settings" screen?
| 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"; | ||
| } |
There was a problem hiding this comment.
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.
| 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"; | ||
| } |
There was a problem hiding this comment.
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.
|
I'm going to take a second pass over this once I've figured out the server->manager/preview use addon state (as I will store this build progress information in addon state). @weeksling I will leave this PR so you can base any UI work on it but be aware I am going to change quite a lot of things later (probably in a distinct PR). |
|
This was merged as part of #69 with a lot of changes. |
Using the new
onTaskProgressoption, switch to a more full featuredBUILD_PROGRESSevent chromaui/chromatic-cli#805.📦 Published PR as canary version:
0.0.42--canary.55.36a4f81.0✨ Test out this PR locally via:
npm install @chromaui/addon-visual-tests@0.0.42--canary.55.36a4f81.0 # or yarn add @chromaui/addon-visual-tests@0.0.42--canary.55.36a4f81.0