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

axetroy
Copy link
Contributor

@axetroy axetroy 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
Copy link
Contributor Author

axetroy commented Jun 19, 2019

/cc @ry
sorry. my mistake.

installer/mod.ts Outdated 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, '\\"')}"`);
Copy link
Member

@ry ry Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it works.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can leave out the anonymous function I think.

Copy link
Contributor Author

@axetroy axetroy Jun 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@ry ry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ry ry merged commit 4317af1 into denoland:master Jun 19, 2019
@axetroy axetroy deleted the installer_args_fix branch June 19, 2019 14:17
ry pushed a commit to ry/deno that referenced this pull request Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants