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

fix: installed module can not get arguments correctly #510

Merged
merged 4 commits into from Jun 19, 2019

Conversation

2 participants
@axetroy
Copy link
Contributor

commented Jun 19, 2019

The generated command is not wrapped in double quotes

This will cause the parameters passed to the script to be incorrect

change:

- deno run --allow-net --allow-read https://deno.land/std/http/file_server.ts hello world "$@"
+ "deno" "run" "--allow-net" "--allow-read" "https://deno.land/std/http/file_server.ts" "hello world" "$@"

axetroy added some commits Jun 19, 2019

@axetroy

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

/cc @ry
sorry. my mistake.

Show resolved Hide resolved installer/mod.ts Outdated
@@ -153,7 +153,7 @@ async function generateExecutable(
filePath: string,
commands: string[]
): Promise<void> {
commands = commands.map((v): string => '"' + v + '"');
commands = commands.map((v): string => `"${v.replace(/\"/g, '\\"')}"`);

This comment has been minimized.

Copy link
@ry

ry Jun 19, 2019

Contributor

Hrm... I don't think this is sufficient to escape the strings properly. (I can't immediately think of a counter example...) What about using JSON.stringify() on the strings?

  commands = commands.map(JSON.stringify);

This comment has been minimized.

Copy link
@axetroy

axetroy Jun 19, 2019

Author Contributor

yes, it works.

This comment has been minimized.

Copy link
@ry

ry Jun 19, 2019

Contributor

You can leave out the anonymous function I think.

This comment has been minimized.

Copy link
@axetroy

axetroy Jun 19, 2019

Author Contributor

@ry No, it can't. typescript compiler will throw an error.

@ry

ry approved these changes Jun 19, 2019

Copy link
Contributor

left a comment

LGTM

@ry ry merged commit 4317af1 into denoland:master Jun 19, 2019

5 checks passed

denoland.deno_std Build #20190619.23 succeeded
Details
denoland.deno_std (Linux) Linux succeeded
Details
denoland.deno_std (Mac) Mac succeeded
Details
denoland.deno_std (Windows) Windows succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@axetroy axetroy deleted the axetroy:installer_args_fix branch Jun 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.