Feature: esm support #797
Feature: esm support #797
Conversation
|
@cressie176 Add esm support |
|
@cressie176 If you want I can also add commitlint to standardize commit messages |
package.json
Outdated
| ], | ||
| "author": "Michael Bridgen <[email protected]>", | ||
| "engines": { | ||
| "node": ">=10" |
There was a problem hiding this comment.
are you sure this will work with older Node versions?
Please don't, it has nothing to do with ESM. It may make sense to do, but that's a whole separate conversation to be had. |
| This package, "amqplib", is licensed under the MIT License. A copy may | ||
| be found in the file LICENSE-MIT in this directory, or downloaded from | ||
| http://opensource.org/licenses/MIT. | ||
| Permission is hereby granted, free of charge, to any person obtaining |
There was a problem hiding this comment.
the project had two license files github does not index LICENSE-MIT I changed it to LICENSE
There was a problem hiding this comment.
The license file follows the MIT file
|
As a security measure, all nodejs runtime packages start with node:util, for example, this kills support for nodejs 10 and 12, and versions like 14, 16, 18, 20, 22 work normally. |
|
You can check again, I believe it is ready to be merged. |
|
@cressie176 The versions that lose support are 10 and 12 for security reasons all nodejs packages now use the prefix "node" example node:util |
|
@cressie176 Versions 10 and 12 are the least used, if you consider most packages no longer support it. |
|
@luas10c As was previously mentioned, it's fine to only support Node 14+ |
|
@kibertoad I know it was a radical change, but it's the way to organize the project for future contributions. If it were part of eslint, it would take longer to solve my problem, which in this case would be support for esm. |
|
Eslint with prettier helped a lot in moving from commonjs to esm and in building for distribution. |
|
using the cli that I sent in a few minutes, maybe a few hours you can see that most of the changes were not added lines, it was just the change of directory from lib to src to standardize the way that what is source is not to be distributed. it is the codebase that will go through polyfill |
But it doesn't even have typescript |
|
@kibertoad @cressie176 I can send 3 PR, one for eslint and prettier, one to change the folder structure and a last one for esm support, what do you say? If you analyze and approve it quickly. |
|
@luas10c Can we do only the ESM one, and discuss the rest? with the advent of biome and oxlint, I'm not sure eslint/prettier are the best option on the market |
|
@luas10c can you hold off for a bit please. I've been reflecting on amqplib and thinking it could be a good time to modernise, but separate from the esm support change.
I would want to approach these as a series of PRs, but once done they would hopefully make adding ESM easier |
Ditto |
|
@cressie176 strong +1 on ditching jest. node:test is excellent, but we can also consider vitest, which is great and would have almost no migration curve. then again, with Claude Code etc converting to node:test should be trivial as well |
|
I agree the vitest is a good candidate, but prefer node's inbuilt one because it is one fewer dependency to worry about. I'm hoping Claude will take care of the grunt work |
|
I agree the PR is too large and mixes several concerns, and that this can be irritating. I also appreciate and want to recognise the substantial work that @luas10c has put in. When comments include phrases like “wtf”, use punctuation marks for emotional emphasis, or make judgments about an author’s understanding, I feel uncomfortable because I need a respectful, concrete review to keep the contributors feeling welcome. Please keep future comments respectful and constructive, centred on specific, actionable changes. Thank you |
The idea of using Jest is more to increase the number of people who have knowledge, which facilitates future contributions. Biome isn't fully customizable like eslint with Prettier. |
Jest performed better in my tests. |
|
@luas10c biome replaces prettier entirely, it has built-in formatter, which supports the most essential aspects of customization. What do you mean by "performed better"? jest isn't faster than node:test |
Jest is massively overcomplicated for what amqplib needs and comes with all sorts of negatives like slow startup and rewriting require / import statements (as I think you've discovered). The last time I used it, it continued running tests after the before function had failed, filling the terminal with unnecessary noise. The concurrent design meant it buffers output. Not only did this lead to noticeably slower test feedback, but it also meant adding logging to debug asynchronous test failures caused by missing awaits became problematic. It is at the bottom of my list of test frameworks I would ever introduce. ESlint is much better, but has problematic upgrades and frequent breaking changes. It is also the #1 source of dependabot noise. Hence why I would like to evaluate alternatives, even if they are less configurable. |
|
Node's Test Runner was introduced in v16.17.0 so we wouldn't be able to test in v14. I would still consider it though. |
The old eslint had a big disadvantage when used on many files, in the case of amqplib it is not enough, the new eslint has a much higher performance. |
Eslint supports several plugins and you can even create custom plugins. Biome does not yet have this support and consumes much more processor. |
|
@luas10c biome also finishes linting most projects in under a second. eslint is not even close perf-wise and I doubt we need custom plugins. |
I don't see Biome as an alternative; it has too many problems. Biome is very fast, and there's another competitor that's also very fast, both made in Rust. |
|
@luas10c what problems do you have in mind? we use biome in Lokalise from everything, it works just fine |
|
I see that if the lib were large with several files it would make a lot of sense, as it is small eslint will not have many problems. eslint is more mature than oxlint and biome |
|
@luas10c This sounds vague. What specific issues would we have with biome? |
Biomejs has more problems than eslint https://github.com/biomejs/biome/issues problems take longer to solve Using eslint and jest is more for the community which is much larger, you will be able to get rid of errors more easily. |
I'm not sure there's very much point going on with this conversation @luas10c. You clearly have strong opinions which may be optimum in the context of your projects, but are very different to the opinions I hold, for the reasons I have explained previously. I'm not adopting Jest, and would prefer to try alternatives before adopting eslint. I'm going to close this PR now because I don't feel it's progressing, and gone off topic from the original purpose. |
3 participants
This PR contains some important changes to the package, one of which is highly anticipated is support for esm modules.
Add esm support with @swc/core transform files
Tested versions were 14.x, 16.x, 18.x, 20.x, 22.x