Add Tasks component #896

matteodepalo · 2022-12-12T16:21:40Z

WHY are these changes introduced?

We want to be able to convey to the user that a some tasks are running. So far we've been using listr2, but we want to follow our own design approach.

~~We'll need to merge this other PR first before we can review this properly.~~

WHAT is this pull request doing?

This PR introduces a new renderTasks method which takes an array of items in the form of: {title: string, task: Promise} and displays an animated bar while these tasks are running. At the end it displays a green bar in case of success or a red bar in case any of the tasks has failed to complete.

How to test your changes?

Run kitchen-sink and see the bar for yourself at the end! It should look like this:

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes
I've made sure that any changes to dev or deploy have been reflected in the internal flowchart.

github-actions · 2022-12-12T16:22:12Z

Thanks for your contribution!

Depending on what you are working on, you may want to request a review from a Shopify team:

Themes: @shopify/theme-developer-tools
UI extensions: @shopify/ui-extensions-cli
Hydrogen: @shopify/hydrogen
Other: @shopify/cli-foundations

github-actions · 2022-12-14T13:11:32Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	68.18% (+0.42% 🔼)	3551/5208
🟡	Branches	63.98% (+0.42% 🔼)	1455/2274
🟡	Functions	66.5% (+0.25% 🔼)	907/1364
🟡	Lines	69% (+0.43% 🔼)	3383/4903

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / Tasks.tsx	100%	100%	100%	100%
🟢	`...` / use-async-and-unmount.ts	100%	50%	83.33%	100%
🟢	`...` / use-layout.ts	100%	83.33%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / ui.tsx	55.56% (-6.94% 🔻)	75%	62.5% (-8.93% 🔻)	55.56% (-6.94% 🔻)
🟢	`...` / ui.ts	100%	75% (-25% 🔻)	100%	100%

Test suite run success

886 tests passing in 459 suites.

Report generated by 🧪jest coverage report action from 9f1d971

pepicrft · 2023-01-03T10:34:38Z

packages/cli-kit/src/private/node/ui/components/TextAnimation.tsx

+import React, {useEffect, useRef, useState} from 'react'
+import gradient from 'gradient-string'


Not blocking, but these imports, especially React, might have a negative impact on the startup time of the CLI. I'm working on a tool that will help us see if this is a bottleneck, and if it is we'll have to move them to dynamic imports.

This PR removes chalk-animation in favor of a more minimal dependency on gradient-string so in theory this should improve things. I'll wait for the decision regarding dynamic imports 👍

pepicrft · 2023-01-03T10:36:01Z

packages/cli-kit/src/private/node/ui/components/Prompt.test.tsx

@@ -34,7 +34,8 @@ describe('Prompt', async () => {

    expect(renderInstance.lastFrame()).toMatchInlineSnapshot(`
      "?  Associate your project with the org Castile Ventures?
-      [36m✔[39m  [36msecond[39m"


When I see these tests I always wonder if we should do matches without ASCI escape characters. As a code reviewer, [36m says nothing to me. I'd have to go and search what the escape code means. Nothing actionable in for this PR though, just a random thought that I had.

The reason why I added these is that if someone changes the color of the border in the prompt this test will fail. It's not really so that developers understand what this is, it's more to protect against breaking changes. See it as a screenshot diff if you will. Without these codes all the prompt tests would look the same.

That's a good idea. In a future iteration we could provide a custom matcher that outputs in the terminal the two outputs so that you can see the styles applied.

Actually if you get an error right now it already works, you will see the colored output in the terminal since the escape sequences are interpreted.

pepicrft · 2023-01-03T10:37:43Z

packages/cli-kit/src/private/node/ui/hooks/use-async.ts

+    asyncFunction()
+      .then(() => {
+        onResolve()
+        unmountInk()


What does unmounting mean in the context of a component?

Also, that side effect doesn't seem well captured in the name of the function, useAsync.

Basically, this hook allows you to wait inside react components for async behavior. Once this behavior resolves we want to unmount the rendering ink instance explicitly so that the promise we're awaiting from the outside (waitForExit) resolves.

Regarding the naming I'm open to suggestions. Would something like useAsyncAndUnmount be better?

@pepicrft how about useEffectAsync?

I'm more inclined towards useAsyncAndUnmount.

pepicrft · 2023-01-03T10:42:42Z

packages/cli-kit/src/private/node/ui/hooks/use-layout.ts

+
+const MIN_WIDTH = 80
+
+export default function useLayout() {


What about adding a unit test for this one?

Will do 👍

pepicrft · 2023-01-03T10:44:32Z

packages/cli-kit/src/private/node/ui/components/Tasks.tsx

+    for (const task of tasks) {
+      setCurrentTask(task)
+      // eslint-disable-next-line no-await-in-loop
+      await task.task()


Are there plans to implement a version of this component with tasks that run concurrently?

I looked at listr2 and it seems like tasks are not being processed concurrently there as well so for now I think we can keep this sequential. Parallel tasks would also need a different interface to report progress.

pepicrft · 2023-01-03T10:52:36Z

packages/cli-kit/src/private/node/ui/hooks/use-async.ts

+
+export default function useAsync(
+  asyncFunction: () => Promise<unknown>,
+  {onResolve = () => {}, onReject = () => {}}: Options,


Nitpick: What about aligning the naming of the then function: onFulfilled and onRejected?

pepicrft · 2023-01-03T10:54:02Z

packages/cli-kit/src/public/node/ui.tsx

+/**
+ * Runs async tasks and displays their progress to the console.
+ */
+export function renderTasks(tasks: Task[]) {


Task is a public interface so I recommend moving it to the public/node folder as well. Otherwise we might introduce breaking changes there in the future without realising it.

renderPrompt also uses PromptProps which is an interface defined privately. We could remove the dependency on these private interfaces by creating new public ones that mirror the private ones, but this might introduce some differences in the future that typescript will not catch for us (e.g. we add a field to the private ones and forget to update the public one).

github-actions · 2023-01-04T11:12:38Z

Benchmark report

The following table contains a summary of the startup time for all commands.

Status	Command	Baseline	Current	Diff
🟢	`app build`	1501 ms	1290 ms	-14.06 %
🟢	`app deploy`	1632 ms	1327 ms	-18.69 %
🟢	`app dev`	1559 ms	1315 ms	-15.65 %
🟢	`app env pull`	1564 ms	1277 ms	-18.35 %
🟢	`app env show`	1499 ms	1246 ms	-16.88 %
🟢	`app generate extension`	1522 ms	1276 ms	-16.16 %
🟢	`app generate schema`	1534 ms	1254 ms	-18.25 %
🔴	`app info`	1501 ms	1911 ms	27.32 %
🟢	`app scaffold extension`	1488 ms	1248 ms	-16.13 %
🟢	`theme check`	1477 ms	1209 ms	-18.14 %
🟢	`theme delete`	1449 ms	1212 ms	-16.36 %
🟢	`theme dev`	1454 ms	1202 ms	-17.33 %
🟢	`theme help-old`	1499 ms	1205 ms	-19.61 %
🟢	`theme info`	1510 ms	1228 ms	-18.68 %
🟢	`theme init`	1486 ms	1234 ms	-16.96 %
🟢	`theme language-server`	1478 ms	1222 ms	-17.32 %
🟢	`theme list`	1454 ms	1221 ms	-16.02 %
🟢	`theme open`	1449 ms	1219 ms	-15.87 %
🟢	`theme package`	1464 ms	1230 ms	-15.98 %
🟢	`theme publish`	1518 ms	1231 ms	-18.91 %
🟢	`theme pull`	1430 ms	1227 ms	-14.2 %
🟢	`theme push`	1494 ms	1215 ms	-18.67 %
🟢	`theme share`	1486 ms	1210 ms	-18.57 %
🟢	`webhook trigger`	1432 ms	1539 ms	7.47 %

matteodepalo force-pushed the progress-bar branch from 4a8b299 to c5f1830 Compare December 14, 2022 12:57

matteodepalo marked this pull request as ready for review December 14, 2022 14:47

This comment has been minimized.

Sign in to view

matteodepalo force-pushed the progress-bar branch 2 times, most recently from 682aaf9 to b394ee5 Compare December 16, 2022 10:01

matteodepalo changed the title ~~Progress bar~~ Loading bar Dec 16, 2022

matteodepalo requested review from a team, pepicrft and gonzaloriestra and removed request for a team December 16, 2022 11:30

matteodepalo force-pushed the progress-bar branch 2 times, most recently from f1d7727 to 09110a8 Compare December 16, 2022 14:35

matteodepalo changed the title ~~Loading bar~~ Add Tasks component Dec 16, 2022

isaacroldan approved these changes Jan 3, 2023

View reviewed changes

pepicrft approved these changes Jan 3, 2023

View reviewed changes

matteodepalo force-pushed the progress-bar branch from fa942e9 to 71a3586 Compare January 3, 2023 11:00

matteodepalo added 12 commits January 4, 2023 10:48

Add Tasks component to replace listr

860f825

Add useLayout hook

440a72c

Fix TextAnimation not clearing timeout properly

cc166d9

Add useAsync hook

e92c5a0

Add more meaningful messages

12c2a1a

Add comment above renderTasks

ed46f33

Progress bar tweaks

cc9c1c7

Remove chalk-animation with custom implementation

0fb9c35

Remove cross in case of failure

cda4450

Add changeset

80c6638

Fix version of gradient-string

4279058

Add final state tests for tasks component

e5cb16f

matteodepalo added 9 commits January 4, 2023 10:48

Remove CI specific code from use-async

bf3b6d1

Tweak task runner styles

a0ee69d

Update tasks tests

f7f3314

Add more padding to the stack trace

3e6e46b

Update error tests

427b196

Add test for useLayout

5777dd6

Rename useAsync to useEffectAsync

01f85fa

Add spaces around url in Link component

09cf409

Update fetch test

b04defa

matteodepalo force-pushed the progress-bar branch from 3ec97ef to b04defa Compare January 4, 2023 10:48

pepicrft approved these changes Jan 4, 2023

View reviewed changes

matteodepalo added 2 commits January 4, 2023 11:00

Rename use-effect-async

554dd13

Update check-version test with new link

9f1d971

matteodepalo merged commit f100055 into main Jan 4, 2023

matteodepalo deleted the progress-bar branch January 4, 2023 11:21

shopify-shipit bot temporarily deployed to production January 5, 2023 11:06 Inactive

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Tasks component #896

Add Tasks component #896

matteodepalo commented Dec 12, 2022 •

edited

Loading

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 14, 2022 •

edited

Loading

This comment has been minimized.

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023

pepicrft Jan 4, 2023

matteodepalo Jan 4, 2023

pepicrft Jan 3, 2023

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023 •

edited

Loading

matteodepalo Jan 3, 2023

pepicrft Jan 4, 2023

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023

pepicrft Jan 3, 2023

pepicrft Jan 3, 2023

matteodepalo Jan 3, 2023

github-actions bot commented Jan 4, 2023

		import React, {useEffect, useRef, useState} from 'react'
		import gradient from 'gradient-string'

Add Tasks component #896

Add Tasks component #896

Conversation

matteodepalo commented Dec 12, 2022 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Measuring impact

Checklist

github-actions bot commented Dec 12, 2022

github-actions bot commented Dec 14, 2022 • edited Loading

Coverage report

Test suite run success

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteodepalo Jan 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 4, 2023

Benchmark report

matteodepalo commented Dec 12, 2022 •

edited

Loading

github-actions bot commented Dec 14, 2022 •

edited

Loading

matteodepalo Jan 3, 2023 •

edited

Loading