Skip to content
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

CLI: show progress/file paths while unpacking/unminifying #82

Open
0xdevalias opened this issue Dec 21, 2023 · 14 comments
Open

CLI: show progress/file paths while unpacking/unminifying #82

0xdevalias opened this issue Dec 21, 2023 · 14 comments
Labels
enhancement New feature or request scope: cli

Comments

@0xdevalias
Copy link

Currently the CLI doesn't seem to show any indication of progress while unpacking (possibly also while unminifying?):

⇒ npx @wakaru/cli unpacker unpacked --unpacker-output stage2-unpacked --perf

┌   Wakaru CLI v0.0.2
│
└  Selected features: Unpacker

┌   Unpacker
│
◇  Unpacking...

It would be nice if we were able to see some more output here; potentially a progress bar, and/or list of the file paths being processed. This would help with understanding what the CLI is currently doing, if it's frozen, etc.

It would also make it easier to identify what file was likely the cause of an error like this, as we could then manually correlate the path of the file being processed with the error message:

SyntaxError: Leading decorators must be attached to a class declaration. (1:5)
    at constructor (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:18018:23)
    at FlowParserMixin.raise (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:20914:23)
    at FlowParserMixin.parseDecorators (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30573:22)
    at FlowParserMixin.parseStatementLike (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30370:29)
    at FlowParserMixin.parseStatementLike (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:22799:28)
    at FlowParserMixin.parseModuleItem (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30349:21)
    at FlowParserMixin.parseBlockOrModuleBlockBody (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30976:40)
    at FlowParserMixin.parseBlockBody (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30969:14)
    at FlowParserMixin.parseProgram (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30248:14)
    at FlowParserMixin.parseTopLevel (/Users/devalias/.npm/_npx/adacf4dfcf214674/node_modules/@wakaru/cli/dist/chunk-KOQHKJQW.cjs:30238:29) {
  code: 'BABEL_PARSER_SYNTAX_ERROR',
  reasonCode: 'UnexpectedLeadingDecorator',
  loc: Position { line: 1, column: 5, index: 5 },
  pos: 5
}

There may be other ways to improve error outputs like that as well (eg. potentially being able to inform the parser of the path/filename of the code being processed, and then it may expose that info directly in the error messages)


It appears that the CLI is currently built with clack:

That seems to have the concept of a spinner, but seemingly not a progress bar/etc:

It can apparently even do multiple tasks in spinners:

From a quick google (in no particular order):

@0xdevalias 0xdevalias changed the title CLI: show progress while unpacking CLI: show progress/file paths while unpacking Dec 21, 2023
@pionxzh
Copy link
Owner

pionxzh commented Dec 21, 2023

Unpacking cannot provide any progress because of the design. Unminify will have a better output on the spinner with --concurrency(Not sure why lol). This can be further improved.

For progress, how about things like (21/37)? Btw, users should be able to see the ... animation to tell the CLI is not frozen?

For error handling, most of error reporting has been slightly improved to print the file and rule name. I will look into the error you provided, do you have a sample?

@pionxzh pionxzh added enhancement New feature or request scope: cli labels Dec 21, 2023
@0xdevalias 0xdevalias changed the title CLI: show progress/file paths while unpacking CLI: show progress/file paths while unpacking/unminifying Dec 21, 2023
@0xdevalias
Copy link
Author

0xdevalias commented Dec 21, 2023

Unpacking cannot provide any progress because of the design.

@pionxzh What aspect of the design prevents it?

The main CLI file calls into the ./unpacker file:

log.step('Unpacking...')
const timing = new Timing()
const { result: items, time: elapsed } = await timing.measureTimeAsync(() => unpacker(inputPaths, outputPath))
log.step('Finished')

Which loops through the provided paths:

export async function unpacker(
paths: string[],
outputDir: string,
): Promise<UnpackerItem[]> {
fsa.ensureDirSync(outputDir)
const result: UnpackerItem[] = []
const files: string[] = []
for (const p of paths) {
const source = await fsa.readFile(p, 'utf-8')

Could that not print out the paths with console.log or similar on each loop as they're being processed?

And then, if a progress bar was to be implemented, for the unpacker, at a basic level, I would think that it could show progress based on the number of paths processed vs the total number of paths to be processed; because at this point, we've already expanded all of the globs.


For progress, how about things like (21/37)? Btw, users should be able to see the ... animation to tell the CLI is not frozen?

@pionxzh Something like (21/37) would also work for my needs. Though ideally I would still like to be able to see the path of each file being processed as well; at the very least if I pass some kind of -v (verbose) type flag.

I'll have to check again, but I don't think the ... was animated for me on the unpack step. There was definitely a spinner after the ... on the unminify step though.


Unminify will have a better output on the spinner with --concurrency (Not sure why lol)

@pionxzh Haha true. I haven't tried that one yet.

It looks like you're not currently using clack's tasks spinners here yet, I wonder if that would improve things further?

log.step(`Unminifying... ${c.dim(`(concurrency: ${concurrency})`)}`)
const s = spinner()
s.start('...')
const timing = new Timing()
const pool = new FixedThreadPool<UnminifyWorkerParams, Measurement>(concurrency, unminifyWorkerFile)
const unminify = async (inputPath: string) => {
const outputPath = path.join(outputDir, path.relative(commonBaseDir, inputPath))
const result = await pool.execute({ inputPath, outputPath, moduleMeta, moduleMapping })
s.message(`${c.green(path.relative(cwd, inputPath))}`)
return result
}
const { result: measurements, time: elapsed } = await timing.measureTimeAsync(() => Promise.all(
unminifyInputPaths.map(p => unminify(p)),
))
pool.destroy()
s.stop('Finished')


For error handling, most of error reporting has been slightly improved to print the file and rule name.

@pionxzh Ah, good to know :)


I will look into the error you provided, do you have a sample?

@pionxzh Unfortunately not currently, aside from saying that it happened while unpacking this folder (Ref) (though that also had other 'uncommitted noise' in it at the time I got that error)

I was going to open a separate issue for it as soon as I could narrow down the source that caused it.

@pionxzh
Copy link
Owner

pionxzh commented Dec 21, 2023

I was wrong. Yes, we can have a progress on unpacker.

I have checked clack's task when I implement the cli, but I will check again.

@pionxzh
Copy link
Owner

pionxzh commented Dec 30, 2023

I removed the spinner in unpacker because of a bug from upstream bombshell-dev/clack#176 .

It looks like you're not currently using clack's tasks spinners here yet, I wonder if that would improve things further?

Clerk's task can only accept a list of task, but the CLI is using a more complex way to manage the tasks (worker pool, concurrency...). Since I already patched clerk by myself for path completion, I can try to enhance task to fulfill the need we want.

@0xdevalias
Copy link
Author

0xdevalias commented Jan 2, 2024

I removed the spinner in unpacker because of a bug from upstream natemoo-re/clack#176 .

@pionxzh That makes sense. Seems there is a PR (linked to that thread) that might fix that:


Clack's task can only accept a list of task, but the CLI is using a more complex way to manage the tasks

@pionxzh Based on a quick skim of the code, I don't think that will matter will it?

log.step(`Unminifying... ${c.dim(`(concurrency: ${concurrency})`)}`)
const s = spinner()
s.start('...')
const timing = new Timing()
const pool = new FixedThreadPool<UnminifyWorkerParams, Measurement>(concurrency, unminifyWorkerFile)
const unminify = async (inputPath: string) => {
const outputPath = path.join(outputDir, path.relative(commonBaseDir, inputPath))
const result = await pool.execute({ inputPath, outputPath, moduleMeta, moduleMapping })
s.message(`${c.green(path.relative(cwd, inputPath))}`)
return result
}
const { result: measurements, time: elapsed } = await timing.measureTimeAsync(() => Promise.all(
unminifyInputPaths.map(p => unminify(p)),
))
pool.destroy()
s.stop('Finished')

The unminify async function wraps the pool.execute; and Promise.all is used to wait on all of the promises after mapping the file paths.

My thought was that instead of Promise.all, it should (probably/hopefully) be possible to just use Clack's tasks, something like:

  import {
    // ..snip..
+   tasks,
    // ..snip..
  } from '@clack/prompts'
  
  // ..snip..
  
- const { result: measurements, time: elapsed } = await timing.measureTimeAsync(() => Promise.all(
+ const { result: measurements, time: elapsed } = await timing.measureTimeAsync(() => tasks(
      unminifyInputPaths.map(p => unminify(p)),
  ))

  // ..snip..

@pionxzh
Copy link
Owner

pionxzh commented Jan 2, 2024

Based on a quick skim of the code, I don't think that will matter will it?

It will try to await and print the task one by one which might be still ok even though we have other task controller. But it won't show user the correct current progress if the first task is slow or something. And the output is also not ideal, there is always a gap between tasks.

@0xdevalias
Copy link
Author

It will try to await and print the task one by one

@pionxzh Oh true.. that's less than ideal. I didn't look deeply into it's implementation earlier, but it seems it's pretty simple:

/**
 * Define a group of tasks to be executed
 */
export const tasks = async (tasks: Task[]) => {
	for (const task of tasks) {
		if (task.enabled === false) continue;

		const s = spinner();
		s.start(task.title);
		const result = await task.task(s.message);
		s.stop(result || task.title);
	}
}; 

I haven't looked deeply at how spinner works, but maybe could just implement our own version of tasks that creates all the spinners together?

@pionxzh
Copy link
Owner

pionxzh commented Jan 6, 2024

I just updated the log of unpacker as a workaround before clack fixed the spinner issue.

For the progress bar, I will try in a few days.

◇  Output directory path (<enter> to accept default)
│  ./out
│
◇  Unpacking test-52.js
│
◇  Unpacking test-98.js
│
◇  Unpacking test-53.js
│
◇  Finished
│
◆  Successfully generated 1470 modules (2m6s)

@0xdevalias
Copy link
Author

Sounds good, thanks :)

@0xdevalias
Copy link
Author

Just came across this lib, looks interesting:

@pionxzh
Copy link
Owner

pionxzh commented Feb 6, 2024

The animation looks good. But I'm afraid we cannot estimate the time haha.

Currently, the CLI will somehow failed to show the current processing file because of the communication between workers and the main thread. This will need some workarounds.

@0xdevalias
Copy link
Author

By my read of that library; it learns to magically estimate the time.

@pionxzh
Copy link
Owner

pionxzh commented Feb 6, 2024

it estimate the time based on history execution results.

@0xdevalias
Copy link
Author

0xdevalias commented Feb 7, 2024

Yup. Which, depending on how it maps those values, would likely give relatively useful estimates when (as an example) unminimising the same project's builds over time (eg. I suspect the _app.js bundle of the project I am usually unminimising will take a similar time for each new build)

Whereas obviously for a completely different project the estimates may be way off.

This method logs a progress bar and estimated duration for a promise. It requires at least two parameters– a Promise and a label (e.g. "Running tests"). The label is SHA1 hashed in order to uniquely identify the promise.

An optional third parameter can be provided as well with the following keys:

name type Description
estimate Number Estimated duration of promise. (This value is used initially, until a history of actual durations have been recorded.)
id String Uniquely identifies the promise. This value is needed if the label string is not guaranteed to be unique.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request scope: cli
Projects
None yet
Development

No branches or pull requests

2 participants