Nice!

Any idea why CI fails though? Even Node 12.x looks to be failing although it looks green in the UI: https://github.com/cloudamqp/amqp-client.js/actions/runs/5595186642/jobs/10235542199?pr=68#step:14:12

The browser test requires the cloudamqp/websocket-tcp-relay container or something similar running next to the AMQP server. I didn't attempt to integrate this into the GitHub actions.

This sounds worthwhile to me but could be a follow-up PR, or I could try to help here, once the other issues are sorted.

I pushed some updates to this pull request.

The "onerror is not called when conn is closed by client" test never fails for me, but I'm running Node.js on Windows, and rabbit-mq in a Docker container. This test has failed previously in this repo, so I wonder if this is version dependent on something?

There's not a lot I seem to be able to do about Node.js 12, it is way out of support now and the libraries specifically don't support it.

Node.js 14 is also out of support, but the libraries support it, so it should work in general, but it looks like a "logical OR assignment" isn't getting transformed properly by Typescript [or esbuild], since this operator was only supported by Node.js 15 and newer. I'll look more to see if there's a specific version of ts-node or Typescript that works with Node.js 14. Older versions still didn't transform correctly during test runs, and setting the Vitest build.target config also didn't work. I ended up patching the code to the transformed output of that line.

I also added a potential solution for the onerror being called when the client is closing the connection. Also calling socket.destroy() in closeSocket() results in the server's TCP RST response to be ignored, while previously it would only allow a FIN response as a non-error.

For updating the GitHub Action workflow to test in the browser, I created a separate branch, ngbrown-forks:bug/55-actions, see diff here.

I used docker compose which will make it simpler to start the development services locally as well.

Here is a successful run: https://github.com/ngbrown-forks/amqp-client.js/actions/runs/5603992318

Nice work making everything green. It should be fine dropping Node 12, maybe even Node 14. I think we should do that in an explicit PR though.

I would like to split up this PR so it becomes more clear what the actually fixes are. Can you open a new PR with just the test improvements? I assume that would show one or more failing tests then? We could mark them as pending/skipped and merge it, and the have the PR with the fixes that would remove the pending/skip from those tests.

@dentarg I can rebase and reorder the commits in this pull request to go in a better sequence. You seem to be asking for a separate pull request every few commits.

Each pull request will need to stack on top of the previous pull requests because they can't work independently. As the pull requests get merged and the follow-on PRs get rebased, they will get simpler, with less to review.

Is this what you want me to do? This could be the PR sequence:

• Limit to Node.js 14 Node.js 16 and above.
• Switch to testing with RabbitMQ and websocket-tcp-relay hosted by docker compose.
• Switch test framework, add browser tests, and test browser in CI.
• This PR, which fixes ArrayBuffer errors with large amounts of messages #55.

The third step seems like a lot, and spitting it up more is possible, but it is most cohesive together. It could even be combined with the previous step because the reason for it existence is otherwise doubtful.

Yes, exactly, and it sounds fine to me to combine step 2 and 3 into one PR

Normally changing the minimum Node.js version in a library will be a breaking change and major version increase. So I might as well drop Node.js 14 as well, since it is also past end-of-life.

Sounds good

I've fully rebased this pull request and rewrote the description.

And finally, this is rebased with just the fix and passing tests.