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 bug: Interactive mode -> Unpacker -> Input file path -> 'Input does not exist' when using prefix/**/*.js glob pattern #81

Closed
0xdevalias opened this issue Dec 21, 2023 · 8 comments · Fixed by #90
Labels
bug Something isn't working scope: cli

Comments

@0xdevalias
Copy link

0xdevalias commented Dec 21, 2023

Trying the new CLI in interactive mode, it seems to have issues when I give it an input file path like unpacked/**/*.js, even though that path definitely exists (running from the root dir of this checked out repo (Ref))

⇒ npx @wakaru/cli

┌   Wakaru CLI v0.0.2
│
│  Run "wakaru --help" for usage options
│
◇  Select features to use (Use <space> to select, <enter> to submit)
│  Unpacker - Unpacks bundled code into separated modules
│
└  Selected features: Unpacker

┌   Unpacker
│
▲  Input file path (Supports glob patterns)
│  unpacked/**/*.js_
└  Input does not exist

If I change the input path to something like unpacked/, then the error message changes to Input is not a file.

While I haven't looked too deeply into it, I think there might currently be a check to ensure that the provided input is a file; but that would then break the ability to import folders and globs.


While it's OOS of this issue, I also noticed that it's not possible to tab complete files/paths while using the interactive mode. That may be by design, but it would probably make it more user friendly. I can open this as a separate issue if desired.

Edit: See:

@0xdevalias 0xdevalias changed the title CLI bug: Interactive mode -> Input file path -> 'Input does not exist' when using prefix/**/*.js glob pattern CLI bug: Interactive mode -> Unpacker -> Input file path -> 'Input does not exist' when using prefix/**/*.js glob pattern Dec 21, 2023
@0xdevalias
Copy link
Author

While I haven't looked too deeply into it, I think there might currently be a check to ensure that the provided input is a file; but that would then break the ability to import folders and globs.

Looking at the code, this seems to be implemented here:

let inputPaths = _inputPaths
if (_inputPaths.length === 0) {
const rawInputPath = await text({
message: `Input file path ${c.dim('(Supports glob patterns)')}`,
placeholder: './input.js',
validate(value) {
if (!value) return 'Please enter a file path'
const inputPath = path.resolve(value)
if (!fsa.existsSync(inputPath)) return 'Input does not exist'
if (!fsa.statSync(inputPath).isFile()) return 'Input is not a file'
if (!isPathInside(cwd, inputPath)) return 'Input is outside of the current working directory'
return undefined
},
})

It seems to be using the existSync / statSync functions from fs-extra:


I'm not sure if the CLI is designed to be able to be passed a folder without a glob, but if so, then these checks should accept a folder as the path.

For checking globs, I think maybe a function like isGlobPath or similar should be added to:

It seems globby is the glob matching library used, which seems to already expose a method that could be used for this isDynamicPattern:

@0xdevalias
Copy link
Author

I'm not sure if the CLI is designed to be able to be passed a folder without a glob, but if so, then these checks should accept a folder as the path

The CLI calls unpacker here:

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

unpacker has a paths arg, which seems to be processed as a list of full file paths; so folders aren't currently supported at this level:

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')


That said, if we wanted to enhance things to be able to specify a folder path (that would then apply a default glob, eg. $folderPath/**/*.js), that should be fairly easy to do. Off the top of my head, we would:

  • check if the provided path is to a folder
  • if so, check if the folder is within the 'allowed working directory'
  • if so, append the default glob pattern to it (eg. **/*.js)
  • allow the normal glob expansion code to process it

@0xdevalias
Copy link
Author

I think currently, the isPathInside check will fail here when given a glob/etc as well:

const inputPath = path.resolve(value)
if (!fsa.existsSync(inputPath)) return 'Input does not exist'
if (!fsa.statSync(inputPath).isFile()) return 'Input is not a file'
if (!isPathInside(cwd, inputPath)) return 'Input is outside of the current working directory'

At least, depending on how that glob is defined I suppose:

/**
* Check if base path contains target path
*/
export function isPathInside(base: string, target: string): boolean {
const relative = path.relative(base, target)
return !relative.startsWith('..') && !path.isAbsolute(relative)
}

@pionxzh
Copy link
Owner

pionxzh commented Dec 21, 2023

I also noticed that it's not possible to tab complete files/paths while using the interactive mode. That may be by design, but it would probably make it more user friendly. I can open this as a separate issue if desired.

This one is on my todo list. Can open in another one.

@pionxzh
Copy link
Owner

pionxzh commented Dec 21, 2023

The path validation in unpacker is not correct because it forgot to calculate the glob.

And this is path validation in unminify:

const resolvedPaths = resolveGlob(value)
if (resolvedPaths.length === 0) return 'No files matched'
if (!isPathInside(cwd, value)) return 'Input is outside of the current working directory'

@pionxzh
Copy link
Owner

pionxzh commented Dec 21, 2023

My expectation on the path input would be only "file(s)" or "glob to files". I'm also not sure about the use of a passing folder...

If we don't change the design, we can improve the error message to be Input is a folder, if you want to specific files in the folder, please use {folder}/**/*.js.

@0xdevalias
Copy link
Author

This one is on my todo list. Can open in another one.

@pionxzh Done:


My expectation on the path input would be only "file(s)" or "glob to files". I'm also not sure about the use of a passing folder...

If we don't change the design, we can improve the error message to be Input is a folder, if you want to specific files in the folder, please use {folder}/**/*.js.

@pionxzh I don't have strong feelings either way. The first way I tried to do it was just passing the input folder path, but it's not much extra effort for me to be explicit and use a glob.

If a folder path on it's own isn't allowed, then I definitely think the improved error message is a good idea.

For simplicity sake/reducing the number of options, I would probably suggest just forcing the user to use a glob + showing the improved error message.

@0xdevalias
Copy link
Author

0xdevalias commented Dec 21, 2023

Whichever way is chosen though, the validation should probably be shared between passing args via CLI flags, versus using interactive mode, as currently it looks like it (at least somewhat) works to pass a folder as the input path via CLI args:

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

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

┌   Unpacker
│
◇  Unpacking...
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
}

Technically even if the user is forced to use a glob, they could write a glob that only matches a folder, so we would also need to make sure that the expanded glob path is filtered down to the list of files (assuming that isn't already being done).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working scope: cli
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants