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: binaries installed in invalid path in Node.js 20 #8

Conversation

sounisi5011
Copy link
Collaborator

In Node.js 20, the behavior of the process.exit() function has changed.
As a result, npm 7.19.0 or later does not set the correct exit code on error.
See: npm/cli#6399
Therefore, the child_process.exec() function will not return an error.

This causes binaries to be installed in the wrong path. For example, in the case of go-task, it will be installed in a path such as:

.../node_modules/@go-task/cli/Unknown command: "bin"

To see a list of supported npm commands, run:
  npm help/task

This is the filepath. A multi-line string filepath like this is used.

See: go-task/task#1190

This happens when npm 91 is installed with Node.js 20.
However, the same problem may occur if the npm bin command fails with other errors (for example, if Corepack does not allow npm to run).

This bug was fixed in npm 9.6.7, but not all users are running the latest version of npm.
In particular, in the environment of users using yarn or pnpm, npm will remain at the old version built into Node.js and will not be updated to the new one.
So the getInstallationPath() function also needs to work around this bug.

This pull request modifies the getInstallationPath() function to avoid this npm bug.

Footnotes

  1. The Unknown command: "bin" error occurs because the npm bin command has been removed from npm 9.

@sounisi5011
Copy link
Collaborator Author

sounisi5011 commented May 31, 2023

As noted in the code comments, this pull request determines the error message by "whether or not the trimmed stdout is an absolute path". This approach is not foolproof, but should work in most cases.


Another approach is to fix the process.exit() function by passing the --require option to the node command. The NODE_OPTIONS environment variable can be used to specify additional arguments to pass to the node command.
To do this, first write the following code in fix-node20-process-exit.cjs:

const origExit = process.exit;
Object.defineProperty(process, 'exit', {
  value: function(...args) {
    if (args[0] !== undefined || typeof process.exitCode !== 'number' || process.exitCode === 0) {
      origExit.apply(this, args);
    }
  },
});

Then, run the npm bin command like this:

$ NODE_OPTIONS="${NODE_OPTIONS} --require /path/to/fix-node20-process-exit.cjs" npm bin

This approach is more reliable than parsing stdout to detect failures of the npm bin command.

However, the process.exit() function has been intentionally fixed. Future npm commands may fail due to this workaround.

Also, fix-node20-process-exit.cjs cannot be combined into the same file at build time, so it must be generated dynamically or the build process must be changed. The former would have a negative impact on performance, and I have determined that the latter is beyond the scope of changes that should be made in this pull request.

Therefore, I did not choose this approach.

@andreynering andreynering merged commit 357090b into go-task:master Jun 14, 2023
@andreynering
Copy link
Member

Thanks @sounisi5011!

@sounisi5011 sounisi5011 deleted the fix-bug/binaries-installed-in-invalid-path-in-nodejs-20 branch June 15, 2023 03:31
sounisi5011 added a commit to sounisi5011/package-version-git-tag that referenced this pull request Jul 2, 2023
The `@go-task/go-npm` included in `@go-task/cli` now installs the `task` command in the correct location on Windows!
see go-task/go-npm#5, go-task/go-npm#8, and go-task/go-npm#9

This reverts commit abb45e1
sounisi5011 added a commit to sounisi5011/package-version-git-tag that referenced this pull request Jul 2, 2023
* ⬆️ Update dependency @go-task/cli to v3.27.1 ( 77618ab )

* 🔥 Remove patches for `@go-task/go-npm` package ( d626085 )

    The `@go-task/go-npm` included in `@go-task/cli` now installs the `task` command in the correct location on Windows!
    see go-task/go-npm#5, go-task/go-npm#8, and go-task/go-npm#9

    This reverts commit abb45e1

* ⬆️ Update `pnpm-lock.yaml` ( 36c5f37 )

* 📝 Update CHANGELOG ( 766d804 )

---------

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Sonishi Izuka <sounisi5011@users.noreply.github.com>
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.

2 participants