Skip to content
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

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

abarisain
Copy link

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

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
.github/workflows/test.yml Outdated Show resolved Hide resolved
@abarisain
Copy link
Author

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

@Nevon
Copy link
Member

Nevon commented Aug 31, 2023

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.

@abarisain
Copy link
Author

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.

@MateuszWicherski
Copy link

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!)

@abarisain
Copy link
Author

It's on my todolist, I hope to be able to wrap this up next week

@abarisain
Copy link
Author

sorry for making some noise on this pr, please bear with me while I fix the tests :)

@abarisain abarisain closed this Sep 29, 2023
@abarisain abarisain reopened this Sep 29, 2023
@abarisain
Copy link
Author

abarisain commented Sep 29, 2023

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.
16, 18, 20 work alright. https://github.com/abarisain/kafkajs-zstd/actions/runs/6349945183/job/17248941953#logs

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?

@jdrakes
Copy link

jdrakes commented Nov 14, 2023

@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.

@abarisain
Copy link
Author

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:

const zstd = eval('import("cppzst")') as Promise<any>;

export function KafkaZstdCodec() {
  return {
    async compress(encoder: any) {
      return (await zstd).compress(encoder.buffer);
    },

    async decompress(buffer: any) {
      return (await zstd).decompress(buffer);
    },
  };
}

the eval looks bad but I did not find any other way to tell TypeScript not to replace import with require.

@jdrakes
Copy link

jdrakes commented Nov 14, 2023

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:

const zstd = eval('import("cppzst")') as Promise<any>;

export function KafkaZstdCodec() {
  return {
    async compress(encoder: any) {
      return (await zstd).compress(encoder.buffer);
    },

    async decompress(buffer: any) {
      return (await zstd).decompress(buffer);
    },
  };
}

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.

@pandzicivan
Copy link

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.

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 npm ci on node 14 breaks te test?

@abarisain
Copy link
Author

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

@abarisain
Copy link
Author

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.
Nor do I really care about this being merged as I just reintegrated the lib into the project, it's just a couple of lines

@JSimoni42
Copy link

@Nevon can you integrate these changes? @abarisain's CI tests passed in his fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants