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

Remove protobufjs in favor of protobuf-es #60

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

talentlessguy
Copy link
Contributor

@talentlessguy talentlessguy commented Feb 8, 2025

This closes #31

Switches to a more correct, maintained and up to date library - protobuf-es. It's more strict so a lot of things are failing. And I also switched from v2 to v3 protobufs because some tooling didn't work properly with it.

WIP

Progress:

yarn test --test-reporter=tap | grep -E '^# tests|^# pass|^# fail'
# tests 258
# pass 246
# fail 12

things that are failing

symlinks:

 actual: Uint8Array(10) [
      10,   8,   8,  4, 18,
       2, 104, 105, 24,  2
    ],
    expected: Uint8Array(8) [
      10, 6,   8,   4,
      18, 2, 104, 105
    ],

raw files with no content:

    actual: Uint8Array(6) [ 10, 4, 40, 0, 56, 0 ],
    expected: Uint8Array(6) [ 10, 4, 8, 0, 24, 0 ],

there's some missing default value in all of those but i'm not sure where

@@ -23,13 +23,14 @@
"gen:types": "tsc --build",
"prepare": "tsc --build",
"test:web": "playwright-test test/**/*.spec.js --cov && nyc report",
"test:node": "c8 --check-coverage --branches 95 --functions 83 --lines 94 mocha test/**/*.spec.js",
"test:node": "c8 --check-coverage --branches 95 --functions 83 --lines 94 node --test test/**/*.spec.js",
Copy link
Member

@alanshaw alanshaw Feb 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does changing the test framework need to happen in this PR? This looks like quite a large PR to review anyway. It would be easier to review if it didn't include changes not related to the goal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mocha breaks with ESM for whatever reason, I didn't want to waste time figure it out so I switched to built-in node test framework while it's a draft

@alanshaw
Copy link
Member

@talentlessguy you might gain more savings switching to protons and protons-runtime instead? It would dudupe with protons-runtime in ipfs-unixfs-exporter.

@alanshaw
Copy link
Member

Note: I think in general I'd prefer to switch to protobuf-es as it seems to have a big community and usage behind it.

@talentlessguy
Copy link
Contributor Author

@talentlessguy you might gain more savings switching to protons and protons-runtime instead? It would dudupe with protons-runtime in ipfs-unixfs-exporter.

thanks for the suggestion

@achingbrain
Copy link
Member

FWIW, last time I ran the benchmark suite, protons was just over 20% faster than protobuf-es when serializing/deserializing - https://github.com/ipfs/protons/tree/main/packages/protons-benchmark#usage

These kind of performance characteristics are very important to high traffic deployments like Lodestar so it's unlikely to be switched away from elsewhere.

@talentlessguy
Copy link
Contributor Author

FWIW, last time I ran the benchmark suite, protons was just over 20% faster than protobuf-es when serializing/deserializing - https://github.com/ipfs/protons/tree/main/packages/protons-benchmark#usage

do you mean protobufjs, and not protons?

@achingbrain
Copy link
Member

No, I mean protons:

image

protobuf.js is also quite a bit faster than protobuf-es but it's CJS and the stack is ESM-only so that rules it out.

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.

Remove dependency on protobufjs
3 participants