fix(deno): refactor to avoid prompts during module import (#2216)

[rivy]

Fix for Deno UI/UX prompt issue described in #2216.

[bcoe]

LGTM, have you tested that the top level await works as expected.

[bcoe]

will this work even though yargs is synchronous? (have you tested?).

[rivy]

I believe that you are correct in inferring that this will cause the import mainline code to execute following other purely synchronous imports. But, I believe, all imports will still execute before any module/user code. I've not seen or used any complex import recipes which would need a specific order of execution for the 'Yargs' module.

I am successfully using a fork with this patch in several user scripts, and I can post some examples. But that's just weak, "works on my machine", evidence. What would you think would help prove that this can be used safety without causing any disruptions?

relevant: Unexpected execution order (denoland/deno#14243)

[Altair-Bueno]

Tbh I'm not a fan of this proposed solution. It makes permission errors silent.

I imagine a situation where someone runs a on a CI platform with some environment variables but yargs doesn't crash and the CI pipeline ends with who knows what configuration. This is a hard bug to track down and results on hours debugging what went wrong.

Proposed solution. Lazy evaluation

Instead of preloading the environment object and cwd, run those functions only when asked. This way, not all scripts require those permissions but Deno still asks on those who need them. If TTY is not available, Deno will crash

Example

#!/usr/bin/env -S deno run 
function getcwd() {
  return Deno.cwd()
}

console.log(Math.random() > 0.5 ? getcwd(): '')

Only asks when getcwd() is evaluated

We can simulate a no TTY environment with true | deno run script.js | cat. This will crash if getcwd() is evaluated

[bcoe]

@rivy what are your thoughts on @Altair-Bueno's suggestion? It sounds pretty reasonable to me:

Deno.env.toObject()

could be moved into getEnv.

[Altair-Bueno]

could be moved into getEnv.

Actually, Deno.env.get is a thing. No need to pre-load the environment object¹

---

After reading the code again I also noticed that mainFilename cannot be lazy loaded. What is this variable used for? What's the difference between mainframe and process.cwd? It is not documented on PlatformShim

yargs/lib/typings/common-types.ts

Line 96 in 561fc7a

mainFilename: string;

yargs/lib/platform-shims/deno.ts

Line 79 in 561fc7a

mainFilename: cwd,

Footnotes

1. https://doc.deno.land/deno/stable/~/Deno.env ↩

[rivy]

@bcoe , I'm happy with a revision to lazy evaluation if that works for the rest of your code.

The current implementation also swallows permission denied errors, making them silent. I assumed that was planned graceful degradation. And, not deeply diving into the code, I was not sure about later use of the results of the two permission-requiring operations. So, I proposed this version as essentially a drop-in replacement, minus the UI prompt behavior.

In general, as long as no permission requiring operations are executed within the mainline of imported module code, the noted UI prompting won't appear, ie, deno run https://deno.land/x/yargs@v17.5.1-deno/deno.ts should not prompt. Lazy evaluation should work just as well, and still allows the importer to control permission feedback. But, it would likely force changes to other code in the Yargs project.

I'm happy to help with further changes or testing.

[bcoe]

@rivy could I bother you to rebase. I trust that this fix is working for you, let's land it.

[rivy]

@rivy could I bother you to rebase. I trust that this fix is working for you, let's land it.

@bcoe, rebased and pushed.

This "Works on my PC (TM)" 😄; but I'd hope that someone else would do a cursory test to make sure it's working elsewhere before releasing it. I've got an example deno script (args.ts) on a working branch of my dxx repository which can be run as deno run https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts. It should run without prompts or errors. Since it's a working branch, it is temporary, but I'll keep it up for some time.

deno run https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts --help

 ERR!  * Missing required permissions; re-run with required permissions (`--allow-env`, `--allow-read`)

deno run -A https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts --help

args 0.0.14

Display all arguments.

Usage:
  deno run -A https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts [OPTION..] [ARG..]

Arguments:
  OPTION  OPTION(s); see listed *Options*
  ARG     ARG(s) to display ('shell'-expanded)

Options:
  -0, -z, --zero  {boolean}  * Display $0 (executable)
  -l, --lines     {boolean}  * Display arguments on separate lines

*Logging:
  --log-level LOG_LEVEL
             {string} {choices: "error", "warning", "warn", "note", "info", "debug", "trace"}
             * Set logging level to LOG_LEVEL (overrides any prior setting)
  --silent   {boolean}   * Silent mode; suppress non-error output (sets 'error' level logging)
  --quiet    {boolean}   * Quiet mode; suppress informational output (sets 'warn' level logging)
  --verbose  {boolean}   * Verbose mode; display verbose output (sets 'info' level logging)
  --debug    {boolean}   * Set 'debug' level logging
  --trace    {boolean}   * Set 'trace' (high-detail 'debug') level logging

*Help/Info:
  -h, --help     {boolean}  * Write help text to STDOUT and exit (exit status => 1 if combined with other arguments/options)
  -V, --version  {boolean}  * Write version text to STDOUT and exit (exit status => 1 if combined with other arguments/options)

Examples:
  # ANSI-C strings
    $ deno run -A https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts $'\xe2\x98\x80\ufe0f\u2601\ufe0f\U1f308' is ☀️☁️🌈

  # Bash-like brace, bracket, and advanced file globbing (with NULLGLOB support [for WinOS])
    $ env nullglob=1 deno run -A https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts **/*.{[cm]js,js,ts}

  # Portable, correct, single and double-quoted ARGs
    $ deno run -A https://cdn.jsdelivr.net/gh/rivy/deno.dxx@6200730/eg/args.ts '*' "*" *

* Copyright (c) 2021-2022 * Roy Ivy III (MIT license)

Note that a fresh reload of all dependencies does show some deno warnings about unspecified dependency versions.

deno run -rq https://cdn.jsdelivr.net/gh/rivy-js/yargs@6d3786c/deno.ts

Warning Implicitly using latest version (0.159.0) for https://deno.land/std/testing/asserts.ts
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/path/mod.ts
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/fmt/printf.ts

In searching through lib\platform-shims\deno.ts, it looks like these are all from incorrectly specified dependencies within 'escalade', 'yargs_parser', and 'y18n'. Just specifying the versions of 'std' used in those will fix the warnings. And the specific modules that they are using from 'std' don't hit the problematic 'prompting' code introduced after v0.134.0, so they can use the most recent version of 'std' without introducing an undesired permission prompt.

$ deno run -q -r "https://deno.land/x/escalade@v3.0.3/sync.ts"
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/path/mod.ts
$ deno run -q -r "https://deno.land/x/yargs_parser@v20.2.4-deno/deno.ts"
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/path/mod.ts
$ deno run -q -r "https://deno.land/x/y18n@v5.0.0-deno/deno.ts"
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/path/mod.ts
Warning Implicitly using latest version (0.159.0) for https://deno.land/std/fmt/printf.ts

I'll create some issues/PRs for the three modules when I get a chance.

[rivy]

@bcoe, I've created the PRs to fix the deno warnings (lukeed/escalade#8, yargs/yargs-parser#464, and yargs/y18n#147).

[rivy]

Ping @bcoe.
Is anything else needed here?

[rivy]

@bcoe,

The PRs yargs/yargs-parser#464, and yargs/y18n#147 look complete. But it looks like @lukeed is concerned about the change to his code (lukeed/escalade#8). I've tried to explain the issue and correct the misperception, but he's ghosted me for the last month.

The included function from escalade is tiny, and easily understood; a corrected version could just as easily be incorporated directly into the code here (with a back-reference to his code, of course). I'm happy to set up a PR to incorporate the yargs-parser and y18n updates as well as a vendored version of the one function from escalade which should (🤞🏻) fix all the Deno warnings.

Let me know...

[bcoe]

I'm happy to set up a PR to incorporate the yargs-parser and y18n updates as well as a vendored version of the one function from escalade which should (🤞🏻) fix all the Deno warnings.

Hey @rivy, the Deno implementation in yargs has very much been meant as an experiment, with the primary use case still being Node.js.

I'm hesitant to vendor additional code into yargs, purely for the benefit of Deno (I'd rather carry the warning for the time being until you figure things out with @lukeed. Is there perhaps a compromise where you pin to a range rather than a specific build?

[rivy]

I'm happy to set up a PR to incorporate the yargs-parser and y18n updates as well as a vendored version of the one function from escalade which should (🤞🏻) fix all the Deno warnings.

Hey @rivy, the Deno implementation in yargs has very much been meant as an experiment, with the primary use case still being Node.js.

I'm hesitant to vendor additional code into yargs, purely for the benefit of Deno (I'd rather carry the warning for the time being until you figure things out with @lukeed. Is there perhaps a compromise where you pin to a range rather than a specific build?

I understand this is a small hassle, but there are problems.

First, @lukeed hasn't responded to a request for more discussion (for more than 6 weeks). So, figuring it out with him seems to be quite a low probability occurrence.

Second, Deno differs substantially from NodeJS in that it doesn't use versions in the "semantic version" sense. Deno uses them as simple tags pinning a particular commit set as a "version". There is no implied semantics or ordering in the tag "version" names. The latest one is the "latest version" no matter the tag name (or prior tag names). So, no such thing as a version range would exist.

And, last, the warning is an actual problem, not just a casual warning. By not setting a version, the imported module is free floating without any guardrails, importing whatever chronologically latest version has been uploaded. It's an open door for a future failure arising from the dependency itself, even with no local code changes. Fully specifying a dependency is the recommended best practice, including recs by the Deno team itself. Not specifying the version is an anti-pattern/code smell.

For any production grade application, all the dependencies must be versioned. There is no exception to this. Keeping it un-versioned would download the latest code, and it may not be compatible with current application code.

Never use un-versioned dependencies

ref: https://medium.com/deno-the-complete-reference/dependency-management-in-deno-48f1c91ad84d

... it is a warning that you are implicitly using a version which is risky. You are welcome to ignore the warning.

• kitsonk

however, it's fairly standard practice to import standard libraries using the latest version isn't it?

Not really. All imports should always be versioned.

• lucacasonato

ref: denoland/deno#10822

For risk management and to promote robust code, I wouldn't import any code with this type of warning as a dependency into my code and strongly recommend to others that it's a bad idea.

As for vendoring this code, you wouldn't be changing anything but the Deno platform shim code. And, currently, you're using two differing versions of escalade anyway, escalade@v3.0.3 for the Deno shim and "escalade": "^3.1.1" for the JS code.

This is the entirety of the code used/imported into the deno platform shim...

import { dirname, resolve } from 'https://deno.land/std/path/mod.ts'

export type Callback = (directory: string, files: string[]) => string | false | void;

function toItems(dir: string) {
	let list = [];
	for (let tmp of Deno.readDirSync(dir)) {
		list.push(tmp.name);
	}
	return list;
}

/** Requires `allow-read` permission. */
export default function (start: string, callback: Callback) {
	let dir = resolve('.', start);
	let stats = Deno.statSync(dir);

	if (!stats.isDirectory) {
		dir = dirname(dir);
	}

	while (true) {
		let tmp = callback(dir, toItems(dir));
		if (tmp) return resolve(dir, tmp);
		dir = dirname(tmp = dir);
		if (tmp === dir) break;
	}
}

Adding a version of this code to just the Deno platform shim code would be very easy and be truly only minimally different (I see only three lines requiring minor changes).

Again, I'd like this to be a robust, long-lived Deno module, and I'm happy to help.

And, thanks for taking the time to look at and discuss this issue.