-
-
Notifications
You must be signed in to change notification settings - Fork 12
feat(analyze): add spinner for analysis phases #244
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| import * as prompts from '@clack/prompts'; | ||
| import {styleText} from 'node:util'; | ||
| import type {PhasedRunner, ReportPhase} from '../types.js'; | ||
|
|
||
| const COMPLETE_SYMBOL = styleText('green', '◈'); | ||
| const ERROR_SYMBOL = styleText('red', '◈'); | ||
|
|
||
| function finishPhase( | ||
| spinner: ReturnType<typeof prompts.spinner>, | ||
| title: string, | ||
| symbol: string | ||
| ) { | ||
| spinner.clear(); | ||
| prompts.log.message(title, {symbol, spacing: 0}); | ||
| } | ||
|
|
||
| export function createAnalyzePhasedRunner(): PhasedRunner { | ||
| return async (phases: ReportPhase[]) => { | ||
| for (const {title, run} of phases) { | ||
| const s = prompts.spinner(); | ||
| s.start(title); | ||
| try { | ||
| await run(); | ||
| finishPhase(s, title, COMPLETE_SYMBOL); | ||
| } catch (err) { | ||
| finishPhase(s, title, ERROR_SYMBOL); | ||
| throw err; | ||
| } | ||
| } | ||
| }; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,20 @@ function updateStats( | |
| } | ||
| } | ||
|
|
||
| export async function runPlugin( | ||
| context: AnalysisContext, | ||
| plugin: ReportPlugin, | ||
| seenExtra: Set<string> | ||
| ): Promise<void> { | ||
| const res = await plugin(context); | ||
|
|
||
| context.messages.push(...res.messages); | ||
|
|
||
| if (res.stats) { | ||
| updateStats(context.stats, res.stats, seenExtra); | ||
| } | ||
| } | ||
|
|
||
| export async function runPlugins( | ||
| context: AnalysisContext, | ||
| plugins: ReportPlugin[] | ||
|
|
@@ -34,12 +48,6 @@ export async function runPlugins( | |
| const seenExtra = new Set<string>(extraStats.map((s) => s.name)); | ||
|
|
||
| for (const plugin of plugins) { | ||
| const res = await plugin(context); | ||
|
|
||
| context.messages.push(...res.messages); | ||
|
|
||
| if (res.stats) { | ||
| updateStats(context.stats, res.stats, seenExtra); | ||
| } | ||
| await runPlugin(context, plugin, seenExtra); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nothing changed here right? other than it being extracted into a single-use function probably unnecessary?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. my initial thought was to encapsulate for readability purposes, but i think that you're right. it might be unnecessary. gonna fix this |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| import {describe, it, expect, beforeAll, afterAll} from 'vitest'; | ||
| import {report, ANALYSIS_PLUGINS} from '../analyze/report.js'; | ||
| import {createTempDir, cleanupTempDir, createTestPackage} from './utils.js'; | ||
|
|
||
| let tempDir: string; | ||
|
|
||
| beforeAll(async () => { | ||
| tempDir = await createTempDir(); | ||
| await createTestPackage(tempDir, { | ||
| name: 'mock-package', | ||
| version: '1.0.0', | ||
| type: 'module', | ||
| main: 'index.js' | ||
| }); | ||
| }); | ||
|
|
||
| afterAll(async () => { | ||
| await cleanupTempDir(tempDir); | ||
| }); | ||
|
|
||
| describe('report phased execution', () => { | ||
| it('runs phases in order when phased runner is provided', async () => { | ||
| const executed: Array<{id: string; title: string}> = []; | ||
|
|
||
| await report({ | ||
| root: tempDir, | ||
| phased: async (phases) => { | ||
| for (const phase of phases) { | ||
| executed.push({id: phase.id, title: phase.title}); | ||
| await phase.run(); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| expect(executed).toEqual([ | ||
| {id: 'setup', title: 'Loading project files'}, | ||
| ...ANALYSIS_PLUGINS.map(({id, title}) => ({id, title})) | ||
| ]); | ||
| }); | ||
|
|
||
| it('returns the same shape without a phased runner', async () => { | ||
| const result = await report({root: tempDir}); | ||
|
|
||
| expect(result).toMatchObject({ | ||
| info: { | ||
| name: 'mock-package', | ||
| version: '1.0.0' | ||
| }, | ||
| stats: { | ||
| name: 'mock-package', | ||
| version: '1.0.0' | ||
| }, | ||
| messages: expect.any(Array) | ||
| }); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,20 @@ import type {Codemod, CodemodOptions} from 'module-replacements-codemods'; | |
| import type {ParsedLockFile} from 'lockparse'; | ||
| import type {ParsedCategories} from './categories.js'; | ||
|
|
||
| export interface ReportPhase { | ||
| id: string; | ||
| title: string; | ||
| run: () => Promise<void>; | ||
| } | ||
|
|
||
| export type PhasedRunner = (phases: ReportPhase[]) => Promise<void>; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think this is whats not sitting right with me. it seems over-engineered that we need a concept of a "phased runner", phases, etc. i would expect something much, much simpler: options.interactive; // boolean, from Options
// elsewhere...
const trackProgress = (options, fn) => {
if (!options.interactive) {
return fn();
}
startSpinner();
const result = await fn();
stopSpinner();
return result;
};
// elsewhere again...
await trackProgress(options, async () => {
// do some stuff
}); |
||
|
|
||
| export interface Options { | ||
| root?: string; | ||
| manifest?: string[]; | ||
| src?: string[]; | ||
| categories?: ParsedCategories; | ||
| phased?: PhasedRunner; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is leaking internal implementation detail into the public options. tbh im not sure we need a concept of a "runner" at all. that part may be overengineered. ill have a think and come back to it |
||
| } | ||
|
|
||
| export interface StatLike<T> { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're not gaining much by extracting this. the function definition and call sites are about as many LoC as these two statements