rpc.cjs , rpc.d.ts, rpc.js -> rpc.ts

[marcus-pousette]

• In a CRA5 app, using the latest version of IPFS (0.65) in (that uses js-libp2p-gossipsub 4.x) you will run into undefined behaviours when

import { RPC } from './message/rpc.js'

as it resolves the namespace RPC but not the RPC class (no namespace merging). This leads to RPC.encodeto fail in the sendRpc method. Overriding babel with useNamespace does not fix the issue. I am not sure exactly what it is the issue, is but because rpc.cjs is a CommonJS module imported as an ESM module I think babel fails to compile the code correctly.

• In this PR I use the RC 5.1.0 and replace rpc.cjs, rpc.d.ts, rpc.js with a single rpc.ts file that seems to fix the issue for me.

You might not want to merge this PR directly as it contains breaking changes for CJS and AMD. Nevertheless I hope this will save other developers some time, running into the same issue (since they can temporarily patch their project with this build)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[fryorcraken]

FYI, facebook/create-react-app#12700

[fryorcraken]

You can use craco to fix the issue in your React app https://github.com/waku-org/js-waku-examples/blob/master/relay-reactjs-chat/craco.config.js

[marcus-pousette]

You can use craco to fix the issue in your React app https://github.com/waku-org/js-waku-examples/blob/master/relay-reactjs-chat/craco.config.js

Thank you for the input! Looks like that would solve this issue also (though still a hotfix in some sense).

In the long term, I am concerned that this is though something every frontend developer has to go through to be able to use the IPFS stack in a React app, and would be glad if the boiler plate environment would use as less config specifications as possible.

[fryorcraken]

In the long term, I am concerned that this is though something every frontend developer has to go through to be able to use the IPFS stack in a React app, and would be glad if the boiler plate environment would use as less config specifications as possible.

I agree with you. I believe the blocker is that ipfs/protons is not as performant as protobufjs.

Note for the developers: new hot typescript protobuf ESM library got released: https://buf.build/blog/protobuf-es-the-protocol-buffers-typescript-javascript-runtime-we-all-deserve

[wemeetagain]

new hot typescript protobuf ESM library got released

Thanks for the heads up. We'll be benchmarking performance of the new library.

Ideally we can upstream any performance and security measures we currently use into an upstream protobuf library.

[dapplion]

Note for the developers: new hot typescript protobuf ESM library got released: https://buf.build/blog/protobuf-es-the-protocol-buffers-typescript-javascript-runtime-we-all-deserve

Our needs are bounded by DOS protection here, as we need bounded list at decode time. That's a not specified feature for protobuf unfortunately.

[wemeetagain]

Our needs are bounded by DOS protection here, as we need bounded list at decode time. That's a not specified feature for protobuf unfortunately.

I would hope there would be some upstream protobuf library that wants to be fast and also resilient against DoS attacks.
Definitely we would need feature completeness for our purposes before considering migrating to an upstream library.

[fryorcraken]

Our needs are bounded by DOS protection here, as we need bounded list at decode time. That's a not specified feature for protobuf unfortunately.

do protobufjs and/or ipfs/protons fulfil this requirement already?

[wemeetagain]

do protobufjs and/or ipfs/protons fulfil this requirement already?

No they don't. We've manually edited the generated code to limit the number of "repeated" fields to be read.

[fryorcraken]

do protobufjs and/or ipfs/protons fulfil this requirement already?

No they don't. We've manually edited the generated code to limit the number of "repeated" fields to be read.

Would it make sense to state this requirement and see what the maintainers think?

https://github.com/bufbuild/protobuf-es/issues

[dapplion]

do protobufjs and/or ipfs/protons fulfil this requirement already?

No they don't. We've manually edited the generated code to limit the number of "repeated" fields to be read.

Would it make sense to state this requirement and see what the maintainers think?

https://github.com/bufbuild/protobuf-es/issues

Opened issue bufbuild/protobuf-es#299

[dapplion]

Would it make sense to state this requirement and see what the maintainers think?

Will not be upstreamed bufbuild/protobuf-es#299

[marcus-pousette]

new hot typescript protobuf ESM library got released

Thanks for the heads up. We'll be benchmarking performance of the new library.

Ideally we can upstream any performance and security measures we currently use into an upstream protobuf library.

If you look in the code, e.g. https://github.com/bufbuild/protobuf-es/blob/084c6e4f47b381ad55e030b667907395a779c938/packages/protobuf/src/binary-encoding.ts#L464

one can see that the project is not optimized for performance as for protobufjs. For example there is no abstraction over memory allocation so for node.js you will node take advantage of allocUnsafe. The library is not allocating all memory at once, but in chunks (which is slow). They use "DataView" which is slower than Uint8Array/Buffer usage where you modify bytes manually. I think encoding/decoding with protobuf-es will be 5x to 10x slower than protobufjs just by comparing the different ways they do encoding, decoding and allocations.

If you want to go into micro-optimizations you would want to do a protobufjs or protons rewrite that can deserialize from the Uint8ArrayList directly (And put the DOS protections in there aswell if that is needed)

If you want to be picky about performance one should also consider better benchmarks where V8 runtime optimization are not creating unwanted performance gains. Like this benchmark for this repo uses a Uint8array with 1024 0s as payload. For more realistic test that should be many random uint8arrays with different sizes that is created before the benchmark. Memory allocation is one of the slowest part of encoding. In the protons benchmark they use a static data, here, the same problem (at least it is created before the benchmark is starting). Additionally they use value checks in the benchmark inside the benchmark itself, which actually makes the benchmark hard to interpret. If the benchmark value check would constitute of 95% of the CPU time (who knows?), then all libs will look just as good.

[silkroadnomad]

FYI, facebook/create-react-app#12700

I had this issue and added manually |cjs to all places where it looked like test: /\.(js|cjs|mjs|jsx|ts|tsx)$/,
in my webpack.config. thanks!

[dapplion]

@marcus-pousette If you want to be picky about performance one should also consider better benchmarks where V8 runtime optimization are not creating unwanted performance gains.

There's definitely some gains to be made, but my main concern is security; specifically to bound lists

[wemeetagain]

I believe this issue has been resolved, as well as several of the other points brought up

• we now use upstream protons library
• the codegen produces a rpc.ts
• protons supports codegen-time and runtime limits for all repeated fields