amqp-node / amqplib

AMQP 0-9-1 library and client for Node.JS
https://amqp-node.github.io/amqplib/
Other
3.67k stars 474 forks source link

Convert CommonJS to ESM #721

Closed revosw closed 1 year ago

revosw commented 1 year ago

This commit aims to convert the entire project to ESM from CommonJS. The ESM journey started with specifying { "type": "module" } inside package.json and fixing errors as I went.

Notable changes:

There are currently 12 failing tests.

Technical changes relating to ESM conversion:

revosw commented 1 year ago

This is a big PR, and I totally understand if conversion to ESM is not viable right now. This can instead be taken as inspiration for some later date, and as a testament to how smoothly the conversion was (I began the migration no more than 7 hours ago.)

I am not sure if there are any public metrics on which node version the library consumers are running. When the majority of consumers are running Node.js v14+ (released April 2020), and all 12 tests are passing, this PR can proceed.

revosw commented 1 year ago

To give an idea on which tests are failing:

1) sending messages
   send to queue and consume noAck:
   Uncaught ReferenceError: headers_encoded is not defined
   at encodeBasicProperties (file:///mnt/c/dev/js/amqplib/lib/defs.js:5488:15)

2) sending messages
   send to queue and consume ack:
   Uncaught ReferenceError: headers_encoded is not defined
   at encodeBasicProperties (file:///mnt/c/dev/js/amqplib/lib/defs.js:5488:15)

...4 more tests with the same "not defined" error...

6) heartbeats
   detect lack of heartbeats:
   Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/mnt/c/dev/js/amqplib/test/channel.js)

...3 more tests with the same "Timeout of 2000ms" error...

11) Roundtrip properties
    <BasicProperties> roundtrip:
    » Threw ReferenceError: contentType_len is not defined
    at encodeBasicProperties (file:///mnt/c/dev/js/amqplib/lib/defs.js:5472:22)

12) Content framing
    Adhere to frame max:
    » Threw ReferenceError: contentType_len is not defined
    at encodeBasicProperties (file:///mnt/c/dev/js/amqplib/lib/defs.js:5472:22)
revosw commented 1 year ago

Combining efforts with #712 would complete the es6 migration

cressie176 commented 1 year ago

As mentioned in https://github.com/amqp-node/amqplib/issues/720, I'm still not sure what practical benefits migrating to ESM will have. On the flip side it would make amqplib unusable for older versions of node.

revosw commented 1 year ago

The benefits for this library specifically are merely for the ecosystem to further converge on a single module system. I agree with you that migration is not currently viable, as there are an undocumented amount of people running ESM-incompatible versions of node.