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

feat(cli): Add Deno.mainModule #6180

Merged
merged 5 commits into from
Jun 11, 2020
Merged

feat(cli): Add Deno.mainModule #6180

merged 5 commits into from
Jun 11, 2020

Conversation

Alhadis
Copy link
Contributor

@Alhadis Alhadis commented Jun 8, 2020

This PR exposes the path/URL of the original script that was executed on the command-line (previously available as location.pathname; removed in #5034). It's the moral equivalent of Node's process.argv[1] value, except that it's a URL string instead of an ordinary path. This is because deno run can execute remote files directly, making it necessary to distinguish between /foo/bar and https://deno.land/foo/bar.

Be aware that for deno eval, Deno.mainModule will contain some fabricated location ending in __$deno$eval.ts. I would have set this property to null in such cases, but this is literally the first time I've ever touched Rust, and my attempts were met with confusing and frustrating errors (I guess 15 minutes isn't enough to wrap one's head around the basics of Rust…)

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2020

CLA assistant check
All committers have signed the CLA.

cli/js/lib.deno.ns.d.ts Outdated Show resolved Hide resolved
cli/js/runtime_main.ts Outdated Show resolved Hide resolved
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.

All for this addition but it should be unstable first.

Copy link
Collaborator

@nayeemrmn nayeemrmn left a comment

Choose a reason for hiding this comment

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

We should probably put this behind a blind read permission check like CWD and exec path. This will in a lot of cases leak the user's username, for example.

cli/tests/script_url.ts Outdated Show resolved Hide resolved
@Alhadis Alhadis changed the title feat(cli): Add Deno.scriptUrl feat(cli): Add Deno.mainUrl Jun 8, 2020
@Alhadis
Copy link
Contributor Author

Alhadis commented Jun 8, 2020

Okay, done. Hopefully I did that correctly.

@Alhadis Alhadis requested a review from ry June 8, 2020 17:59
@Alhadis
Copy link
Contributor Author

Alhadis commented Jun 8, 2020

@ry FYI, the tests pass when I run them locally. The CI failure seems to have been caused by a connection timeout.

cli/js/lib.deno.unstable.d.ts Outdated Show resolved Hide resolved
cli/ops/runtime.rs Show resolved Hide resolved
cli/ops/runtime.rs Outdated Show resolved Hide resolved
cli/ops/runtime.rs Show resolved Hide resolved
@Alhadis Alhadis changed the title feat(cli): Add Deno.mainUrl feat(cli): Add Deno.mainModule Jun 11, 2020
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 - thanks!

@ry ry merged commit ca5b5ba into denoland:master Jun 11, 2020
@Alhadis Alhadis deleted the script-url branch June 11, 2020 12:48
@ghost
Copy link

ghost commented Jun 13, 2020

@Alhadis would you mind explaining what the difference is to import.meta.url?

Put in a script file, these two outputs are equivalent:

console.log(Deno.mainModule)
console.log(import.meta.url)

Or is it meant to have a more concise wording?

@kitsonk
Copy link
Contributor

kitsonk commented Jun 13, 2020

It isn't when that module imports another module. They are only equivalent in the main module.

@ghost
Copy link

ghost commented Jun 13, 2020

Right, thanks! I just had the usual case in mind, that the script entry point triggers all main-file related code actions, so import.meta.url would be equivalent here. Deno.mainModule certainly is more flexible and a good addition.

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.

5 participants