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

Get rid of cross-spawn dependency #578

Open
sindresorhus opened this issue Oct 27, 2023 · 6 comments
Open

Get rid of cross-spawn dependency #578

sindresorhus opened this issue Oct 27, 2023 · 6 comments

Comments

@sindresorhus
Copy link
Owner

sindresorhus commented Oct 27, 2023

It's pretty unmaintained.

We do a lot to the shell, so would be simpler to do that too in execa ourselves.

Maybe some of the things it fixes are already fixed in Node.js too.

https://github.com/moxystudio/node-cross-spawn/blob/master/lib/parse.js

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 28, 2023

I agree that it not being maintained is a problem. Some of the bug reports seem rather important and are not currently looked into.

Breaking down cross-spawn feature by feature (which are all fixing problems with Windows):

  1. Executing files that have a shabang (since that's a Unix concept). Workaround for users would be to specify the interpreter explicitly instead. This would not work if the script location relies on the PATH environment variable, as opposed to being a file path. It does so by using which on the command, then reading the first bytes of the file to detect the shabang.
  2. Allows the command to a file path using the Unix path syntax. It does so by using path.normalize(), after having run which on the command.
  3. Some issues with escaping (commands with spaces, etc.) although I am not completely sure whether this is fixed or not in the latest version of Node. cross-spawn seems to fix this by wrapping the command in an additional cmd.exe call and manually escaping the command and arguments, using cmd.exe-specific syntax.
  4. It supports PATHEXT even when the shell option is false.

I am feeling conflicted.

  • On one hand, I am concerned that some of those fixes might be important to some users and any change might break their usage of Execa.
  • On the other hand, the current logic seems rather intricate and apparently buggy (according to the current GitHub issues on that repository), and therefore might be a high burden to maintain ourselves.

What are your thoughts?

@sindresorhus
Copy link
Owner Author

Relevant: bcoe/awesome-cross-platform-nodejs#26

@sindresorhus
Copy link
Owner Author

Maybe we could convince Bun to fix some of the things mention here to force Node.js' hand 🤣

@sindresorhus
Copy link
Owner Author

sindresorhus commented Oct 29, 2023

What are your thoughts?

I think it's worth doing it. cross-spawn is already buggy and unmaintained. We would release it as a major version to reduce the potential impact of any regressions.

@ehmicky
Copy link
Collaborator

ehmicky commented Oct 29, 2023

Yes, that sounds good.

We would also need to add automated tests for the features covered by cross-spawn, although our current tests already cover some of it.

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it. For example, some of it can be:

  • Trimmed, e.g. we do not want the ENOENT logic
  • Simplified
  • Fixed, i.e. by looking into the GitHub issues of cross-spawn

@sindresorhus
Copy link
Owner Author

That's some major undertaking, so maybe we should start by just copy/pasting the code as is in an initial PR, and then start refactoring it.

👍

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