fix: remove setEncoding on write stream

[jhohlfeld]

This PR fixes #681

• remove setEncoding on a Writable stream (break bun runtime)

The reason behind this issue is that Writable.setEncoding is not part of the node API and will break in the bun runtime. Since "utf8" is the default encoding anyways, this setting should be safe to be removed.

[jhohlfeld]

To test this change, install bun and run a minimal example:

package.json

{
  "name": "nodejq-issue",
  "module": "index.ts",
  "type": "module",
  "devDependencies": {
    "@types/bun": "latest"
  },
  "peerDependencies": {
    "typescript": "^5.0.0"
  },
  "dependencies": {
    "node-jq": "^4.3.1"
  }
}

index.ts

import { run as jq } from "node-jq";

jq(".", { foo: "bar" }, { input: "json" }).then(console.log);

Install bun & run the example:

curl -fsSL https://bun.sh/install | bash
bun index.ts

When no change applied, you'll receive this error:

39 |                 if (!promiseAlreadyRejected) {
40 |                     promiseAlreadyRejected = true;
41 |                     return reject(err);
42 |                 }
43 |             });
44 |             process.stdin.setEncoding("utf-8");
                 ^
TypeError: process.stdin.setEncoding is not a function. (In 'process.stdin.setEncoding("utf-8")', 'process.stdin.setEncoding' is undefined)

Once this change is applied:

% bun index.ts               
{
  "foo": "bar"
}

[davesnx]

Sounds good, is there any way we can setup a test in CI to run against bun as well?

[jhohlfeld]

Hey @davesnx. I looked into it. I believe extending your ci matrix for the current job 'build-test' isn't necessary or wise at this point. You don't want to build your library under bun, which also doesn't work.

Do you really want to extend your workflows for testing alternative runtimes at this point?

If so, I would suggest building the lib using npm as-is and just running the tests against the other runtime. I could try to include this in the current matrix. However, I see two issues:

1. never tested bun under windows (should be fine though)
2. another step or two in the same job (build-test) could be difficult to maintain and even mess things up on the long run

An alternative would be to introduce another job specifically for alternative runtimes.

Let me know your thoughts on this.

[jhohlfeld]

By the way, there's a github action oven-sh/setup-bun.

[jhohlfeld]

Running the tests under the bun runtime results in all tests being green but one.

First thing to note is that bun comes with its own test runner. This is fine, 99% of tests work just fine.

npm install
npm run build
bun test
...
1 tests failed:
✗ jq core > should catch errors from child process stdin [17.77ms]

 49 pass
 1 fail
Ran 50 tests across 4 files. [338.00ms]

The problematic test has some runtime specific assertions that fail under bun:

src/jq.test.js:74

        expect(error).to.be.an.instanceof(Error)
        // On Mac/Linux, the error code is "EPIPE".
        // On Windows, the equivalent code is "EOF".
        expect(error.message).to.be.oneOf(['write EPIPE', 'write EOF'])
        expect(error.code).to.be.oneOf(['EPIPE', 'EOF'])
        expect(error.syscall).to.equal('write')

AssertionError: expected 'jq: error: invalid/0 is not defined a…' to be one of [ 'write EPIPE', 'write EOF' ]
✗ jq core > should catch errors from child process stdin [17.77ms]

The error under node:

Error: write EPIPE
    at WriteWrap.onWriteComplete [as oncomplete] (node:internal/stream_base_commons:94:16) {
  errno: -32,
  code: 'EPIPE',
  syscall: 'write'
}

under bun:

error: jq: error: invalid/0 is not defined at <top-level>, line 1:
invalid
jq: 1 compile error

      at node-jq/src/exec.js:21:25
      at emit (node:events:161:95)
      at #maybeClose (node:child_process:787:27)
      at #handleOnExit (node:child_process:583:31)

error.code: undefined
error.syscall: undefined

Bun does implement child_process in its own specific way but is compatible with the node API as far as I can see. However, this seems obvious being a more advanced runtime feature.

Would you consider loosening this specific test to support more runtime implementations, @davesnx?

[jhohlfeld]

Addendum:

actually I find this behavior to be as expected. The exit code in this case is '3':

jq "invalid" src/__test__/fixtures/large.json
jq: error: invalid/0 is not defined at <top-level>, line 1:
invalid
jq: 1 compile error

As a result, the error handling in https://github.com/sanack/node-jq/blob/main/src/exec.js#L17-L26 simply forwards the error state coming from jq. My hypothesis is that bun forwards stdin error responses from child processes as-is, while node treats it in some other ways.

[davesnx]

Let's merge this and open another PR with the bun CI. Breaking tests to support a runtime is a bad idea, I would rather have conditionally different behaviours based on bun inconsistencies than broken code out-there.

[github-actions]

🎉 This PR is included in version 4.3.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀