Tasks list #69
Tasks list #69
Conversation
There was a problem hiding this comment.
Looks good. Besides the small comments I have, I believe the UI needs a bit of improvement. Two things pop in mind:
- There's contention between the package headers (the green ones) and the steps. Once we have the steps, I don't see why we need the package headers. We can have the package headers written in the task header.
- The collapse happens after the phase ends. I believe it's better that it should happen after each step ends.
packages/cli/src/command-build.js
Outdated
| } | ||
|
|
||
| /** @type {( linesToDisplay?: number) => (payload: string, showAll?: boolean) => string} */ | ||
| const StreamBuffer = (linesToDisplay = 10) => { |
There was a problem hiding this comment.
Bilt style prefers function over arrow function, except for cases where the function is trivial (e.g. x => x * 2), and in this case it isn't trivial. I would prefer you convert this to a function.
| /** @type {( linesToDisplay?: number) => (payload: string, showAll?: boolean) => string} */ | ||
| const StreamBuffer = (linesToDisplay = 10) => { | ||
| /** @type {string[]} */ | ||
| let buffer = [] |
There was a problem hiding this comment.
The name confused me, because buffer has a very specific meaning in Node.js. Maybe lines? Or lineBuffer?
There was a problem hiding this comment.
done, It is now LinesBuffer
packages/cli/src/command-build.js
Outdated
| childProcess.stdout.pipe(process.stdout) | ||
| childProcess.stderr.pipe(process.stderr) | ||
| const buffer = StreamBuffer() | ||
| childProcess.stdout.on('data', (payload) => { |
There was a problem hiding this comment.
This part doesn't make sense. Is it true that if an stderr payload arrives, it will clear the existing stdout payload? And vice-versa? (I may be misunderstanding task.output =.
There was a problem hiding this comment.
task.output = is a complete override of the output, the only thing that "has a memory" is the LinesBuffer.
Because both stdout and stderr share the same buffer, when an error occurs it'll just append it to the buffer(and will not slice it to 10 lines) so you can see the entire process
There was a problem hiding this comment.
Right! One array for both. Missed that. Thanks!
packages/cli/src/command-build.js
Outdated
| } | ||
| }, | ||
| skip: async () => { | ||
| const shouldSkip = !(await stepExecution.shouldSkip()) |
We also have a new issue in line 549, the await is not wrapped in try/catch - which can cause unhandled rejections, I still need to decide how to handle those errors. |
|
@reggev As you can see, the build failed. I cloned your PR, and the |
No description provided.