search

ipfs / protons

Protocol Buffers for Node.js and the browser without eval
Other
32 stars 23 forks source link

fix: port protobuf reader/writer to ts #60

Closed achingbrain closed 1 year ago

achingbrain commented 2 years ago

Ports just the parts of protobufjs we use to typescript to avoid the extra deps and integrates the changes from https://github.com/protobufjs/protobuf.js/pull/1557 to have native BigInt support.

achingbrain commented 2 years ago

This cannot be merged until https://github.com/nodejs/node/issues/44186 as resolved as it introduces a performance regression due to the use of small, short-lived ESM classes.

wemeetagain commented 1 year ago

@achingbrain can this be revisited? It seems that the problem was more so related to the style of import (eg: named vs default vs namespace) rather than esm vs cjs.

wemeetagain commented 1 year ago

This will open the door for more performance improvements (eg: using a more optimized varint reader / writer)

This will also make adding additional types more straightforward (eg: 64 bit number types associated with https://github.com/ipfs/protons/issues/112)

wemeetagain commented 1 year ago

This appears to have no performance penalty vs the current master. (node v20.2.0 on linux 5.15 x86_64) current master

pbjs x 8,051 ops/sec ±0.60% (91 runs sampled)
protons x 7,993 ops/sec ±0.45% (91 runs sampled)
protobuf.js x 8,182 ops/sec ±0.71% (91 runs sampled)

this PR

pbjs x 8,081 ops/sec ±0.69% (89 runs sampled)
protons x 8,353 ops/sec ±0.28% (95 runs sampled)
protobufjs x 8,376 ops/sec ±0.29% (95 runs sampled)
github-actions[bot] commented 3 months ago

:tada: This issue has been resolved in version 3.0.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: