Add the exit code to the fatal error thrown from common.error(). #1179

kmashint · 2025-02-12T03:12:11Z

While shelljs.exec(cmd) on fatal errors does have the exit code in the logEntry message, it's not available as a separate property on the thrown Error instance. The only way to find the exit code on a fatal thrown error is to require('shelljs/src/common.js').state.errorCode, an awkward use of an internal.

This adds the code from options.code, also used to set the internal state.errorCode, to the options.fatal error before throwing it, simplifying the ability to shelljs.exit(err.code) when catching the Error. This is similar to how the child_process.exec() has a code on the err object given to its callback, e.g. in src/exec.js execASync().

This updates the package.json to an assumed 0.8.6 since the latest release is 0.8.5.

Something seems to be wrong with node 22.x builds over 22.9, causing errors with npm. I downgraded locally on Win10 to node 22.9 and it ran without this low-level crash.
tinymce/tinymce#10018
From Ubuntu errors, the problem is on the signal handling in foreground-child@2.0.0 index.js where the signal resolves like a string not a number.

TypeError [ERR_INVALID_ARG_TYPE]: The "code" argument must be of type number. Received type string ('128SIGABRT')
    at process.set [as exitCode] (node:internal/bootstrap/node:123:9)
    at ChildProcess.<anonymous> (/home/runner/work/shelljs/shelljs/node_modules/foreground-child/index.js:63:22)

  child.on('close', (code, signal) => {
    // Allow the callback to inspect the child’s exit code and/or modify it.
    process.exitCode = signal ? 128 + signal : code

npm ls foreground-child indicates this comes via the nyc dependency which has a new version 17.1.0 that requires newer foreground-child@3.3.0. But underlying that is spawn-wrap@2.0.0 that has no newer version and uses the problematic foreground-child@2.0.0. I tried forcing some overrides and directly replacing versions but it's still broken with the node 22.14.0, as it is with 22.13.1 in the cloud build. There are also version conflicts where foreground-child@3.3.0 wants signal-exit@3 while spawn-wrap@2 and other dependencies want signal-exit@3.

codecov · 2025-02-12T03:13:38Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.28%. Comparing base (7e71b26) to head (87f3a16).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1179   +/-   ##
=======================================
  Coverage   97.28%   97.28%           
=======================================
  Files          36       36           
  Lines        1361     1364    +3     
=======================================
+ Hits         1324     1327    +3     
  Misses         37       37

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kmashint · 2025-02-13T04:50:13Z

@nfischer I've tried many variations including removing my changes, and it seems the problem is between the shelljs dependencies and node 22.13.1, or perhaps anything beyond v22.9.0, where limiting to node v22.9.0 works locally.

Is it possible limit the node 22 could tests to 22.9.0 until the deeper dependency issues can be resolved?
Part of the problem is related to getting forground-child@3.3.0 instead of @2.0.0 that has the 128SIGINT bug. nyc@15.1.0 has an update to @17.1.0, but that breaks other platforms so code and or tests needs to be adjusted.

If we can limit the build tests to 22.9.0 then at least some small improvements could be added while the deeper compatibility issues with node@22 are investigated and resolved.

Thanks for your insight and support!

nfischer · 2025-02-14T03:33:25Z

@kmashint can you file a new bug to share details about things being broken on Node 22? This sort of thing happens from time to time, I'll see what I can do to fix it.

I'll look through your PR when I get a chance.

nfischer

Can you please add a unit test for this? test/config.js would be a good place for this. I think you can do something like calling shell.exec('exit 2'), catch the exception, and check that it has the .code property. Remember to unset shell.config.fatal at the end of the test (preferably in a finally block).

package.json

src/common.js

kmashint · 2025-02-14T04:39:37Z

Thanks @nfischer , I've made adjustments to this. I had a successful test with node@22.9.0, 22.10.0 onward have this new compatibility issue, on Ubuntu and OSX related to the 128SIGINT combination that stems from foreground-child@2.0.0 index.js where the signal resolves like a string not a number. As you noted, I'll create a separate issue for this.

TypeError [ERR_INVALID_ARG_TYPE]: The "code" argument must be of type number. Received type string ('128SIGABRT')
    at process.set [as exitCode] (node:internal/bootstrap/node:123:9)
    at ChildProcess.<anonymous> (/home/runner/work/shelljs/shelljs/node_modules/foreground-child/index.js:63:22)

// related to foreground-child/index.js:63
  child.on('close', (code, signal) => {
    // Allow the callback to inspect the child’s exit code and/or modify it.
    process.exitCode = signal ? 128 + signal : code

On Windows I receive a more cryptic error, which I assume is also related to the signal.

C:\WINDOWS\system32\cmd.exe [8364]: void __cdecl node::fs::InternalModuleStat(const class v8::FunctionCallbackInfo<class v8::Value> &) at c:\ws\src\node_file.cc:1040
  #  Assertion failed: (args.Length()) >= (2)

kmashint · 2025-02-15T01:00:11Z

I haven't worked with Ava tests, but have with Jest and Jasmine. Jasmine has had a resurgence for its simplicity, and the latest Jest is more than 4x the size of Ava. Jasmine is the safest against external dependency conflicts with only one, glob, but I'm not familiar enough with Ava to make comparisons. The latest Ava is down to 180 nested deps, much better than at 1.4.1. Once of the great differentiators of shelljs is the frugality of its dependencies and it covers the common uses with little fuss. I have another PR brewing for Promise support via, say, execAsync(cmd, options) => Promise<ShellString> or throws Error with .code for the exitCode.

$ npm view jasmine@5.6.0 dependencies
{ glob: '^10.2.2', 'jasmine-core': '~5.6.0' }

$ npx howfat jasmine@5.6.0
jasmine@5.6.0 (30 deps, 3.65mb, 416 files, ©MIT)

$ npx howfat ava@6.2.0
ava@6.2.0 (180 deps, 12.19mb, 3318 files, ©MIT)

$ npx howfat ava@1.4.1
ava@1.4.1 (560 deps, 31.72mb, 7635 files, ©MIT)

$ npx howfat jest@29
Warning: error request aborted for https://registry.npmjs.org/graceful-fs
jest@29.7.0 (454 deps, 51.86mb, 9332 files, ©MIT)
├─┬ @jest/core@29.7.0 (359 deps, 42.7mb, 7209 files, ©MIT)

$ npx howfat tape
tape@5.9.0 (221 deps, 9.1mb, 7503 files, ©MIT)

package.json

test/config.js

kmashint · 2025-02-16T20:38:33Z

Sorry for the confusion from a lingering commit from trying different branches. I've cleaned up the commits and added another test variation since the code coverage complained. Tests pass now with the 22.9.0 limiter, and I'll leave it to you to close the conversations after verifying. Thanks!

Dropping support for everything before node v18, which is the current maintenance LTS and also the version in Ubuntu 24.04 LTS. This also drops support for odd-numbered node versions since I only want to officially support Node LTS going forward. Non-LTS node support can be supported by the community if necessary. This also updates all devDependencies while trying to minimize the required source code changes. Some devDependencies can go higher still (ava, chalk), but this is going to require extensive refactoring. Test: npm run test-with-coverage Test: npm run lint Test: npm run check-node-support

nfischer · 2025-02-20T05:43:18Z

Thanks for the contribution! This should be part of the 0.9.0 release when that eventually goes out.

kmashint force-pushed the common-error-throw-with-code branch 6 times, most recently from 5f5d8f2 to 9ac4236 Compare February 13, 2025 04:38

nfischer requested changes Feb 14, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

src/common.js Outdated Show resolved Hide resolved

kmashint mentioned this pull request Feb 14, 2025

nodejs v22.10.0 onward give errors: void node::fs::InternalModuleStat(const v8::FunctionCallbackInfo<v8::Value>&) #1180

Closed

kmashint force-pushed the common-error-throw-with-code branch 3 times, most recently from f1e804a to 5732b8a Compare February 15, 2025 18:26

nfischer requested changes Feb 16, 2025

View reviewed changes

package.json Outdated Show resolved Hide resolved

test/config.js Outdated Show resolved Hide resolved

test/config.js Outdated Show resolved Hide resolved

kmashint force-pushed the common-error-throw-with-code branch 3 times, most recently from 3cdd71f to 1944f81 Compare February 16, 2025 19:58

nfischer and others added 3 commits February 18, 2025 11:51

Add the exit code to the fatal error thrown from common.error().

7611148

Adjust from code review.

04d8819

kmashint force-pushed the common-error-throw-with-code branch 3 times, most recently from 81a73e0 to c735322 Compare February 18, 2025 17:10

Improve test coverage from cloud build complaint.

87f3a16

kmashint force-pushed the common-error-throw-with-code branch from c735322 to 87f3a16 Compare February 18, 2025 20:52

kmashint requested a review from nfischer February 19, 2025 19:53

nfischer approved these changes Feb 20, 2025

View reviewed changes

nfischer merged commit d9d7f4d into shelljs:master Feb 20, 2025
11 checks passed

nfischer added this to the v0.9.0 milestone Feb 20, 2025

nfischer added the feature label Feb 20, 2025

renovate bot mentioned this pull request Mar 20, 2025

Update minor and patch justo-mend/demo-renovate-mcw#13

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add the exit code to the fatal error thrown from common.error(). #1179

Add the exit code to the fatal error thrown from common.error(). #1179

Uh oh!

kmashint commented Feb 12, 2025 •

edited

Loading

Uh oh!

codecov bot commented Feb 12, 2025 •

edited

Loading

Uh oh!

kmashint commented Feb 13, 2025

Uh oh!

nfischer commented Feb 14, 2025

Uh oh!

nfischer left a comment

Uh oh!

Uh oh!

Uh oh!

kmashint commented Feb 14, 2025

Uh oh!

kmashint commented Feb 15, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmashint commented Feb 16, 2025 •

edited

Loading

Uh oh!

Uh oh!

nfischer commented Feb 20, 2025

Uh oh!

Uh oh!

Add the exit code to the fatal error thrown from common.error(). #1179

Add the exit code to the fatal error thrown from common.error(). #1179

Uh oh!

Conversation

kmashint commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kmashint commented Feb 13, 2025

Uh oh!

nfischer commented Feb 14, 2025

Uh oh!

nfischer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kmashint commented Feb 14, 2025

Uh oh!

kmashint commented Feb 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmashint commented Feb 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nfischer commented Feb 20, 2025

Uh oh!

Uh oh!

kmashint commented Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

kmashint commented Feb 15, 2025 •

edited

Loading

kmashint commented Feb 16, 2025 •

edited

Loading