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 deno node:process execPath compatibility #1160

Merged
merged 5 commits into from
Oct 16, 2024

Conversation

w3cj
Copy link
Contributor

@w3cj w3cj commented Oct 14, 2024

  • When running execa from Deno the following error occurs:
The "nodePath" option must be a string or a file URL: /Users/username/deno/bin/deno.

Note: This issue currently occurs in the Nuxt CLI when trying to run with Deno: denoland/deno#25779

Simple reproduction (run this with Deno v2):

import { execa } from "npm:execa";

const result = await execa("ls");
console.log({ result });

lib/arguments/file-url.js Outdated Show resolved Hide resolved
@ehmicky
Copy link
Collaborator

ehmicky commented Oct 14, 2024

Looks good to me!

Thanks a lot @w3cj for making this PR and adding information in the PR comment and the code comment. 🙏

Should we merge and release @sindresorhus?

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 14, 2024

Should we merge and release @sindresorhus?

Go ahead 👍

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 14, 2024

I have a few additional thoughts.


First, what Deno is doing here is quite hacky (in their own words). Using an object like this where a string is expected is probably going to create quite many issues: not just that one solved in this PR, but probably with other projects. That being said, I do understand why they might need to do that (that's because process.execPath requires Deno permissions, i.e. must be lazily computed).

I am wondering whether it would be slightly safer to convert those to a string instead?

export const safeNormalizeFileUrl = (file, name) => {
	const fileString = normalizeFileUrl(normalizeDenoExecPath(file));
	// ...
}

// In Deno node:process execPath is a special object, not just a string:
// https://github.com/denoland/deno/blob/f460188e583f00144000aa0d8ade08218d47c3c1/ext/node/polyfills/process.ts#L344
const normalizeDenoExecPath = file =>  isDenoExecPath(file)
	? file.toString()
	: file

const isDenoExecPath = file => typeof file !== 'string'
	&& file
	&& Object.getPrototypeOf(file) === String.prototype;

Then, the following syntax needs to be taken into account too:

execa(...).pipe(process.execPath, ...)

Right now, it relies on the following check, which would not work with Deno:

if (typeof firstArgument === 'string' || firstArgument instanceof URL) {
if (Object.keys(boundOptions).length > 0) {
throw new TypeError('Please use .pipe("file", ..., options) or .pipe(execa("file", ..., options)) instead of .pipe(options)("file", ...).');
}
const [rawFile, rawArguments, rawOptions] = normalizeParameters(firstArgument, ...pipeArguments);
const destination = createNested(mapDestinationArguments)(rawFile, rawArguments, rawOptions);
return {destination, pipeOptions: rawOptions};
}

Adding some ... || isDenoExecPath(firstArgument) should be enough to fix this.


Finally, we should add automated tests for the following cases:

  • execa(process.execPath, ...)
  • execa(...).pipe(process.execPath, ...)
  • execaNode(...)

When process.execPath is like Deno. Instead of running with Deno, we could monkey patch process.execPath to make it look like Deno.

Ideally, we would run the automated tests with Deno, but I don't think Deno compatibility is one of our goals right now since that's quite a big requirement for a project like Execa. But that PR is ok though since it is a rather simple fix.

If you need help with the automated tests, please let me know.


What do you think @w3cj?

@ehmicky ehmicky self-requested a review October 14, 2024 22:14
@w3cj
Copy link
Contributor Author

w3cj commented Oct 15, 2024

Yeah that seems reasonable. I wasn't quite sure what else in the code base would be effected. I should have some time to work on this later today.

@w3cj
Copy link
Contributor Author

w3cj commented Oct 16, 2024

Updated safeNormalizeFileUrl as mentioned and added tests. Please let me know if this is what you had in mind.

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 16, 2024

That's perfect, thanks @w3cj!

@ehmicky ehmicky merged commit f3a1bf3 into sindresorhus:main Oct 16, 2024
8 checks passed
@ehmicky
Copy link
Collaborator

ehmicky commented Oct 16, 2024

Released in 9.4.1.

renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 20, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

#### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 21, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 22, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 23, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 24, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 25, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 26, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 27, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.4.1 |


## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 28, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.5.0 |


## [v9.5.0](sindresorhus/execa@v9.4.1...4d1e55a)



## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 28, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.5.0 |


## [v9.5.0](sindresorhus/execa@v9.4.1...4d1e55a)



## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 30, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.5.1 |


## [v9.5.1](sindresorhus/execa@v9.5.0...c8cff27)



## [v9.5.0](sindresorhus/execa@v9.4.1...4d1e55a)



## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
renovate bot added a commit to andrei-picus-tink/auto-renovate that referenced this pull request Oct 30, 2024
| datasource | package | from  | to    |
| ---------- | ------- | ----- | ----- |
| npm        | execa   | 9.4.0 | 9.5.1 |


## [v9.5.1](sindresorhus/execa@v9.5.0...c8cff27)



## [v9.5.0](sindresorhus/execa@v9.4.1...4d1e55a)



## [v9.4.1](https://github.com/sindresorhus/execa/releases/tag/v9.4.1)

##### Bug fixes

-   Fix using `process.execPath` with Deno. Thanks [@w3cj](https://github.com/w3cj)! ([#1160](sindresorhus/execa#1160))
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.

3 participants