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

Bun support #435

Closed
samuelgja opened this issue May 4, 2023 · 6 comments
Closed

Bun support #435

samuelgja opened this issue May 4, 2023 · 6 comments

Comments

@samuelgja
Copy link

Motivation

Support for new bun runtime

What's wrong

I'am using avsc in node, but starting digging into new bun runtime and I tried to move some of my lib to bun, but avsc has error while using fromBuffer method.
error is:

1 | return function readTesJt(t) {
2 |   return new TesJt(
3 | 
4 |     t0._read(t)
       ^
TypeError: Expected string

Note this error is from bun run test.js script, not node.

I tried to dig into the lib itself and try to find the issue, but it's too big for me :D without knowing context. So maybe it can be easy fix, maybe not. Maybe it's issue with bun and not with avsc.

If it's issue with bun itself, please close.

Code to reproduce

I found out it only happend on string type.

const avro = require("avsc");

const object = {
  //   integer: 1,
  //   float: Math.PI,
  string: "Hello, world!",
  //   array: [10, 20, 30],
  //   map: { foo: "bar" },
  //   timestampExt: 12312312,
};

const type = avro.Type.forSchema({
  type: "record",
  name: "Test",
  fields: [
    // { name: "integer", type: "int" },
    // { name: "float", type: "float" },
    { name: "string", type: "string" },
    // { name: "array", type: { type: "array", items: "int" } },
    // { name: "map", type: { type: "map", values: "string" } },
    // {
    //   name: "timestampExt",
    //   type: { type: "long", logicalType: "int" },
    // },
  ],
});

const buf = type.toBuffer(object); 

const val = type.fromBuffer(buf); 
@mtth
Copy link
Owner

mtth commented May 24, 2023

Hi @samuelgja, thanks for the report. I think the root cause here is Bun's Buffer.utf8Slice; you can reproduce the issue without avsc:

Buffer.from('abc').utf8Slice(0, 1); // Throws TypeError: Expected string

I'm not familiar with Bun but it looks like the argument order here might not match the underlying implementation (which expects the encoding first, same as Node). Consider filing an issue there.

In the meantime, you may be able to work around this by patching the implementation:

Buffer.prototype.utf8Slice = function (start, end) {
  return this.toString('utf8', start, end);
}

@joscha
Copy link
Contributor

joscha commented Sep 23, 2024

#410 might aid this

@joscha
Copy link
Contributor

joscha commented Sep 25, 2024

On latest master (14886e8) you can now use bun (tested with 1.1.29) out of the box.
Try checking out the repo and then:

bun run mocha --ui tdd

You can try it via:

nix develop --command bun run mocha --ui tdd

with the following Nix flake:

{
  description = "Bun workflow";

  inputs = {
    flake-parts.url = "github:hercules-ci/flake-parts";
    nixpkgs.url = "github:NixOS/nixpkgs";
  };

  outputs = inputs@{ self, flake-parts, ... }:
    flake-parts.lib.mkFlake { inherit inputs; } {
      systems =
        [ "x86_64-linux" "aarch64-linux" "x86_64-darwin" "aarch64-darwin" ];
      perSystem = { config, self', inputs', pkgs, ... }: {
        devShells.default = pkgs.mkShell {
          buildInputs = with pkgs; [
            bun
          ];
        };
      };
    };
}

(see joscha@0e568e3)

Given all unit tests pass I'd think this issue can be closed. We could potentially define bun support in the package.json and add a github workflow for it, if @mtth is happy to give it official support in the future.

@mtth
Copy link
Owner

mtth commented Sep 28, 2024

Do tests also run for avsc 5.x on current bun? If so, closing SGTM. If not, let's close when 6.0 is released.

Adding official support will be best done separately if/when there is demand for it.

@joscha
Copy link
Contributor

joscha commented Sep 28, 2024

Yes 5.x (778f8da) passes well:

Screenshot 2024-09-28 at 9 55 44 PM

@mtth
Copy link
Owner

mtth commented Sep 29, 2024

Great, thank you for confirming. Closing.

@mtth mtth closed this as completed Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants