initial poc of adding a dynamic prefix to app dev output #3797
Conversation
|
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
| const parsedLog : ParsedLog = parseLog(chunk.toString('utf8')) | ||
| const prefix = parsedLog.prefix ?? process.prefix | ||
| const index = addPrefix(prefix, prefixes) | ||
|
|
||
| const lines = stripAnsi(parsedLog.log.replace(/(\n)$/, '')).split(/\n/) |
There was a problem hiding this comment.
So we are parsing before splitting lines, do we assume chunk === line?
Could we receive log lines from different functions in the same chunk?
There was a problem hiding this comment.
🤔 I don't think so? Though we could add support for that later I think by looking for the prefix marker at the start of any line?
* add dynamic prefix to some build commands
| }, | ||
| [concurrentColors], | ||
| ) | ||
| const writableStream = useCallback( |
There was a problem hiding this comment.
@isaacroldan The use of useCallback here seemed unnecessary and I ended up removing it while I was iterating on state management for the seen prefixes. I could probably reintroduce it now, but is it actually needed?
| @@ -71,49 +77,95 @@ function currentTime() { | |||
| */ | |||
| const ConcurrentOutput: FunctionComponent<ConcurrentOutputProps> = ({ | |||
| processes, | |||
| prefixColumnSize = 12, | |||
There was a problem hiding this comment.
What should our default minimum be?
| @@ -193,10 +195,14 @@ const Dev: FunctionComponent<DevProps> = ({ | |||
| const season = now.getMonth() > 3 ? 'Summer' : 'Winter' | |||
| const year = now.getFullYear() | |||
|
|
|||
| const maxPrefixLength = Math.max(...errorHandledProcesses.map((process) => process.prefix.length), ...app.extensions.map((extension) => extension.handle.length)); | |||
| const prefixColumnSize = Math.min(maxPrefixLength, 25); | |||
There was a problem hiding this comment.
Where should we put this default max? override by env var?
| [lineColor], | ||
| ) | ||
|
|
||
| const concurrentColors = [ |
There was a problem hiding this comment.
Some of these colors appear too dark in ITerm.
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/ui/components.d.ts@@ -1 +1 @@
-export { ConcurrentOutput } from '../../../private/node/ui/components/ConcurrentOutput.js';
\ No newline at end of file
+export { ConcurrentOutput, prefixLog } from '../../../private/node/ui/components/ConcurrentOutput.js';
\ No newline at end of file
packages/cli-kit/dist/private/node/ui/components/ConcurrentOutput.d.ts@@ -3,10 +3,18 @@ import { AbortSignal } from '../../../../public/node/abort.js';
import { FunctionComponent } from 'react';
export interface ConcurrentOutputProps {
processes: OutputProcess[];
+ prefixColumnSize?: number;
abortSignal: AbortSignal;
showTimestamps?: boolean;
keepRunningAfterProcessesResolve?: boolean;
}
+/**
+ * Allows users of ConcurrentOutput to format logs with a prefix column override.
+ *
+ * @param prefix The override value for the prefix column
+ * @param log The log to prefix
+ */
+declare function prefixLog(prefix: string, log: string): string;
/**
* Renders output from concurrent processes to the terminal.
* Output will be divided in a three column layout
@@ -20,4 +28,4 @@ export interface ConcurrentOutputProps {
* rsnode backend/index.js
*/
declare const ConcurrentOutput: FunctionComponent<ConcurrentOutputProps>;
-export { ConcurrentOutput };
\ No newline at end of file
+export { ConcurrentOutput, prefixLog };
\ No newline at end of file
|
|
Needs unit tests |
|
Need to review other uses of ConcurrentOutput as well |
WHY are these changes introduced?
Fixes #0000
WHAT is this pull request doing?
How to test your changes?
Post-release steps
Measuring impact
How do we know this change was effective? Please choose one:
Checklist
devordeployhave been reflected in the internal flowchart.