-
Notifications
You must be signed in to change notification settings - Fork 6
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
base: main
Are you sure you want to change the base?
Conversation
@@ -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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
@talentlessguy you might gain more savings switching to protons and protons-runtime instead? It would dudupe with |
Note: I think in general I'd prefer to switch to |
thanks for the suggestion |
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. |
do you mean protobufjs, and not protons? |
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:
things that are failing
symlinks:
raw files with no content:
there's some missing default value in all of those but i'm not sure where