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

execPath still requires the use of deno #155

Open
paoloricciuti opened this issue Jul 7, 2023 · 13 comments
Open

execPath still requires the use of deno #155

paoloricciuti opened this issue Jul 7, 2023 · 13 comments

Comments

@paoloricciuti
Copy link

export const execPath: typeof Deno.execPath = () => which.sync("deno");

I'm trying to build a cli with Deno and cross publish to npm using dnt but the cli works only if deno is installed and the above LOC should be the culprit. It tryes to find deno and throws if there's no deno installed.

Can i do something about this?

@dsherret
Copy link
Member

Would you be able to show more details about your problem?

If the code is using Deno.execPath() to get the location of Deno, then this is correct for getting the location of Deno. If the code is using Deno.execPath() to not get the location of Deno, then probably that calling code should be changed.

@paoloricciuti
Copy link
Author

Would you be able to show more details about your problem?

If the code is using Deno.execPath() to get the location of Deno, then this is correct for getting the location of Deno. If the code is using Deno.execPath() to not get the location of Deno, then probably that calling code should be changed.

Sure...i'm writing this cli. It's quite simple and its using this https://deno.land/x/cmd@v1.2.0 to parse Deno.args and call the proper command.

https://github.com/SvelteLab/cli/tree/main/src

For the moment there's just a single command but actually even just trying to run npx sveltelab (that should only show the default message) results in an error when the user doesn't have deno installed.

➜  ~ npx sveltelab save https://www.sveltelab.dev/4n06i8am85quhu0 test
Need to install the following packages:
  sveltelab@0.0.7
Ok to proceed? (y) 
/Users/stanislav.khromov/.npm/_npx/aa144ceddd2d411a/node_modules/which/which.js:121
  throw getNotFoundError(cmd)
  ^

Error: not found: deno
    at getNotFoundError (/Users/stanislav.khromov/.npm/_npx/aa144ceddd2d411a/node_modules/which/which.js:10:17)
    at Function.whichSync [as sync] (/Users/stanislav.khromov/.npm/_npx/aa144ceddd2d411a/node_modules/which/which.js:121:9)
    at Object.execPath (/Users/stanislav.khromov/.npm/_npx/aa144ceddd2d411a/node_modules/@deno/shim-deno/dist/deno/stable/functions/execPath.js:9:40)
    at Object.<anonymous> (/Users/stanislav.khromov/.npm/_npx/aa144ceddd2d411a/node_modules/sveltelab/script/deps_2/deno.land/x/cmd@v1.2.0/deps.js:64:28)
    at Module._compile (node:internal/modules/cjs/loader:1256:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1310:10)
    at Module.load (node:internal/modules/cjs/loader:1119:32)
    at Module._load (node:internal/modules/cjs/loader:960:12)
    at Module.require (node:internal/modules/cjs/loader:1143:19)
    at require (node:internal/modules/cjs/helpers:110:18) {
  code: 'ENOENT'
}

Node.js v18.16.1

This is my build file that uses dnt

https://github.com/SvelteLab/cli/blob/main/tasks/build-npm.ts

@paoloricciuti
Copy link
Author

Ok apparently it's used here

https://deno.land/x/cmd@v1.2.0/deps.ts?source

but given that this is shimming Deno...should this actually return the execPath of node?

@dsherret
Copy link
Member

dsherret commented Jul 11, 2023

It looks like commander is asking for the location of Deno here:

https://github.com/acathur/cmd/blob/07339f4c7f8971c0a86e9371bf686b1549984fbd/deps.ts#L37

So I don't think this is an issue in these shims. It looks like cmd is specifically geared towards Deno and in this case it would need to be updated to handle possibly executing in Node.

What I'd recommend doing is upgrading dnt to the latest 0.37.0 version to get npm specifier support:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/tasks/build-npm.ts#L1

Then change this line:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/src/deps/commander.ts#L1

To something like:

import { Command } from 'npm:commander@^11.0';

That seems to make it work for me.

@paoloricciuti
Copy link
Author

It looks like commander is asking for the location of Deno here:

https://github.com/acathur/cmd/blob/07339f4c7f8971c0a86e9371bf686b1549984fbd/deps.ts#L37

So I don't think this is an issue in these shims. It looks like cmd is specifically geared towards Deno and in this case it would need to be updated to handle possibly executing in Node.

What I'd recommend doing is upgrading dnt to the latest 0.37.0 version to get npm specifier support:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/tasks/build-npm.ts#L1

Then change this line:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/src/deps/commander.ts#L1

To something like:

import { Command } from 'npm:commander@^11.0';

I don't know if you looked at my previous comment but what i'm asking is: if the goal of dnt (and more specifically node_shims) is to provide a way to cross publish deno and npm shouldnt this shim change the execPath from deno to node?

@dsherret
Copy link
Member

dsherret commented Jul 11, 2023

Actually, you could just add the following when calling build to your build-npm.ts:

mappings: {
		'https://deno.land/x/cmd@v1.2.0/mod.ts': {
			name: 'commander',
			version: '^11.0'
		}
	},

@paoloricciuti
Copy link
Author

I think i did that in the beginning but if i'm not mistaken the two packages have slightly different apis...i'll try tho

@dsherret
Copy link
Member

I don't know if you looked at my previous comment but what i'm asking is: if the goal of dnt (and more specifically node_shims) is to provide a way to cross publish deno and npm shouldnt this shim change the execPath from deno to node?

Usually people will use execPath with arguments to launch a Deno application. The arguments are usually Deno specific and have stuff like run --allow-read=. main.ts and translating that to Node is very challenging to figure out.

For example, what would it do in this scenerio?

export function runDeno(args: string[]) {
  const denoExec = Deno.execPath();
  // code here that calls deno with the provided arguments
}

It's better for library authors to use something like https://github.com/dsherret/which_runtime to figure out what runtime the code is being run in for specific scenarios, then add some runtime specific behaviour.

@paoloricciuti
Copy link
Author

paoloricciuti commented Jul 11, 2023

I don't know if you looked at my previous comment but what i'm asking is: if the goal of dnt (and more specifically node_shims) is to provide a way to cross publish deno and npm shouldnt this shim change the execPath from deno to node?

Usually people will use execPath with arguments to launch a Deno application. The arguments are usually Deno specific and have stuff like run --allow-read=. main.ts and translating that to Node is very challenging to figure out.

For example, what would it do in this scenerio?

export function runDeno(args: string[]) {
  const denoExec = Deno.execPath();
  // code here that calls deno with the provided arguments
}

It's better for library authors to use something like https://github.com/dsherret/which_runtime to figure out what runtime the code is being run in for specific scenarios, then add some runtime specific behaviour.

Mmm i guess it's fair...at the end of the day this should mostly be a problem for libraries that are ported from node to deno and try to recreate process in deno. That said i still think a shim should provide the "correct" path for the shimmed version.

However i completely understand and trust you on this decision 😄

Feel free to close this issue if you don't think it's a good idea to change this.

@paoloricciuti
Copy link
Author

It looks like commander is asking for the location of Deno here:

https://github.com/acathur/cmd/blob/07339f4c7f8971c0a86e9371bf686b1549984fbd/deps.ts#L37

So I don't think this is an issue in these shims. It looks like cmd is specifically geared towards Deno and in this case it would need to be updated to handle possibly executing in Node.

What I'd recommend doing is upgrading dnt to the latest 0.37.0 version to get npm specifier support:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/tasks/build-npm.ts#L1

Then change this line:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/src/deps/commander.ts#L1

To something like:

import { Command } from 'npm:commander@^11.0';

That seems to make it work for me.

I'm trying to use this but if i try to build with dnt it throws the follwing error

[dnt] Transforming...
error: Uncaught (in promise) TypeError: Unsupported scheme "npm:" for module "npm:commander@^11.0". Supported schemes: ["data:","blob:","file:","http:","https:"].
    throw new TypeError(
          ^
    at getValidatedScheme (https://deno.land/x/deno_cache@0.2.1/file_fetcher.ts:58:11)
    at FileFetcher.fetch (https://deno.land/x/deno_cache@0.2.1/file_fetcher.ts:229:20)
    at FetchCacher.load (https://deno.land/x/deno_cache@0.2.1/cache.ts:106:30)
    at fetch_specifier (https://deno.land/x/dnt@0.28.0/lib/pkg/snippets/dnt-wasm-a15ef721fa5290c5/helpers.js:6:22)
    at __wbg_fetchspecifier_bcacee1e00bcee02 (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm.generated.js:271:21)
    at <anonymous> (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm_bg.wasm:1:2124302)
    at <anonymous> (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm_bg.wasm:1:1791039)
    at <anonymous> (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm_bg.wasm:1:2715050)
    at <anonymous> (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm_bg.wasm:1:1079734)
    at <anonymous> (https://deno.land/x/dnt@0.28.0/lib/pkg/dnt_wasm_bg.wasm:1:2632650)

is that expected behavior?

@dsherret
Copy link
Member

Try this:

What I'd recommend doing is upgrading dnt to the latest 0.37.0 version to get npm specifier support:

https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/tasks/build-npm.ts#L1

@paoloricciuti
Copy link
Author

Try this:

What I'd recommend doing is upgrading dnt to the latest 0.37.0 version to get npm specifier support:
https://github.com/SvelteLab/cli/blob/6c9074a8912cfa27c51041f028c1c5382a9acec1/tasks/build-npm.ts#L1

Uh sorry didn't saw that. However just as an hjeads up version 0.35.0 and up errors out

error: Uncaught (in promise) TypeError: Deno.Command is not a constructor
    const process = new Deno.Command(cmd, {
                    ^
    at runCommand (https://deno.land/x/dnt@0.35.0/lib/utils.ts:46:21)
    at async build (https://deno.land/x/dnt@0.35.0/mod.ts:210:5)
    at async file:///C:/Users/Utente/Desktop/Vari/sveltelab_cli/tasks/build-npm.ts:12:1

However i was able to build with 0.34.0

@dsherret
Copy link
Member

I think you're using an old version of Deno (deno upgrade will upgrade to the latest).

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

No branches or pull requests

2 participants