-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update cppzst to latest ver #9
Conversation
Thankfully, as this is already fully async we can use the async import This also updates a couple of deps so the project runs on a modern Node (18/20) tests: update kafka to make it work on arm64
570690b
to
d512dc9
Compare
I totally forgot about the CI when making this PR, I'll take a loot at the failures. I only tested the unit tests & package itself in an actual projet using node 18/20 Sorry about that |
No worries. To make the workflow smoother, I think I would suggest setting up your own temporary actions to run in your fork, since I have to approve the workflow runs when they run in the context of this repo. The CI runs seem to work fine in 18 and 20, but fail in 14 for some reason. Seems like it should work in 14, since 14 should have support for dynamic imports, but there's probably something we're missing. |
As it's failing right in "npm ci", I wonder if it is that the "package-lock" is too new for Node 14. Maybe deleting the file would work around the issue I'll test this on my fork as suggested, and add 16. |
Hi @abarisain ! Is there any way you would find some time to finish this PR? I have got the same issue and I am glad You are trying to solve it - thank You for your work (Yours @Nevon as well!) |
It's on my todolist, I hope to be able to wrap this up next week |
sorry for making some noise on this pr, please bear with me while I fix the tests :) |
Well, I fixed the CI: the problem was that node 14's bundled npm is too old to understand the lockfile and crashes. Updating npm to the last version supporting 14 works. But now node 14 segfaults while running the test. I don't know why and I'm not sure I can fix it. As node 14 is EOL, I'd suggest to simply add an "engine" to package.json and then you can release this as a major version. I can do the package json if it's okay with you, and then squash my commits, wdyt? |
@Nevon My team would love to use this library for connecting with Kafka. Anyway we can get this MR approved and deployed? Without this change your library does not work with Node versions greater than 14. |
If this is a blocker, it's quite easy to reintegrate into your project. Just copy paste index.js in a file, and you have it. Funny coincidence: it's what I did today. We use TypeScript and output to commonjs so I had to get a little creative:
the eval looks bad but I did not find any other way to tell TypeScript not to replace import with require. |
Thank you, this unblocked us for the time being. I hope to get the more permanent fix shipped soon. |
Publishing the pkg with new major version seems reasonable 🤷🏻 @abarisain I would love to look in the node:14 issues, but CI logs expired and cannot be seen. Is there any way to reproduce this locally - does |
I think "npm ci" initially failed because of the lockfile version but I downgraded that. I'll see if I can re-trigger the ci on my fork |
A build has been triggered here: https://github.com/abarisain/kafkajs-zstd/actions/runs/7422261627 Please note that don't have any interest in supporting node < 14 in this PR: It's EOL, it's time to move on. |
@Nevon can you integrate these changes? @abarisain's CI tests passed in his fork. |
This PR updates cppzst: using a old version of this lib comes with complications on linux.
I had to update quite a lot to get tests to run on a modern NodeJS (18/20). The use of dynamic imports is required to get around the ESM requirement, as making this an ES Module is quite heavy and has implication on every projet that uses this lib
I am not sure what minimum version of node this requires, but it works on the current LTS: 18.
Tests can also now be ran on arm64 linux.
I've successfully tested these changes in a kafka project