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

Use proto library that provides tree-shakeable code #335

Closed
2 tasks
D4nte opened this issue Nov 30, 2021 · 14 comments
Closed
2 tasks

Use proto library that provides tree-shakeable code #335

D4nte opened this issue Nov 30, 2021 · 14 comments
Assignees
Labels
track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC

Comments

@D4nte
Copy link
Contributor

D4nte commented Nov 30, 2021

Problem

ts-proto generate code that is not tree-shakeable. See status-im/status-web#233

This is an issue as it inflate the package size for library consumers.

It also uses Buffer.

Solution

Possibly investigate using ipfs/protons or protobuf-js directly.

Definition of Done

Bonus

js-waku currently uses bufbuild and ts-proto which has several downsides:

  • bufbuild is a separate binary that has to be installed
  • ts-proto generates code that uses Buffer
    It also has pros:
  • Generated code is TypeScript
  • Generates from proto files

Review whether we can use the solution for the examples in js-waku too.

@D4nte
Copy link
Contributor Author

D4nte commented Jan 29, 2022

With protobufjs it's possible to build without bufbuild

https://github.com/filoozom/tfe-uliege-code/tree/main/platform%2Fnode

@prichodko
Copy link

I've also detected that generated files are not tree-shakeable, leaving a lot of unused code in the final bundle. See more: status-im/status-web#233. I will investigate, whether there are some possibilities to optimize.

@D4nte
Copy link
Contributor Author

D4nte commented Apr 1, 2022

Maintenance has been resumed 🎉 ipfs/protons@74d3b7a

Focusing this issue in the tree-shakeable problem.

@D4nte D4nte changed the title Replace protons with another package Use proto library that provides tree-shakeable code Apr 1, 2022
@D4nte D4nte changed the title Use proto library that provides tree-shakeable code Use proto library that provides tree-shakeable code and not use Buffer May 20, 2022
@fryorcraken fryorcraken changed the title Use proto library that provides tree-shakeable code and not use Buffer Use proto library that provides tree-shakeable code Aug 25, 2022
@fryorcraken
Copy link
Collaborator

fryorcraken commented Aug 25, 2022

Protons is facing some performance issues: ipfs/protons#51
The also haven't accepted the PR to align behaviour change with proto3: ipfs/protons#48

Focus of this issue is still to have a tree-shakeable protbuf code. Whether we use protons, protobufjs (unlikely as it is cjs) or on own fork (of protons)

@fryorcraken fryorcraken added the track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC label Aug 25, 2022
@fryorcraken
Copy link
Collaborator

@fryorcraken
Copy link
Collaborator

Need to check if protons is tree shakeable also check this convo: ChainSafe/js-libp2p-gossipsub#368 (comment)

@fryorcraken
Copy link
Collaborator

Do note status-im/status-web#337

@fryorcraken
Copy link
Collaborator

Definition from https://github.com/vacp2p/waku are used

Most of the work done here: #1196

Peer exchange still pending: waku-org/waku-proto#14

@danisharora099
Copy link
Collaborator

Based on #1245, it can be deduced that protobuf-es while reducing the total lines of code by 50%, still increases the package size by about 13% which makes protons better suited?

cc @fryorcraken

@fryorcraken
Copy link
Collaborator

Based on #1245, it can be deduced that protobuf-es while reducing the total lines of code by 50%, still increases the package size by about 13% which makes protons better suited?

cc @fryorcraken

Ok seems that in terms of bundle size, it does not help. Noting that this issue is specifically about bundle size (tree shakeable code) it would make sense to keep protons.

@danisharora099
Copy link
Collaborator

Based on #1245, it can be deduced that protobuf-es while reducing the total lines of code by 50%, still increases the package size by about 13% which makes protons better suited?
cc @fryorcraken

Ok seems that in terms of bundle size, it does not help. Noting that this issue is specifically about bundle size (tree shakeable code) it would make sense to keep protons.

Correct.

Would this mean we are closing this issue with the above findings?

@danisharora099
Copy link
Collaborator

danisharora099 commented Mar 31, 2023

As a followup to the above thread:

One idea that came up in a discussion with @felicio was if we could have the Logos scaling simulation tests run js-waku with both protobuf libraries and see some real-world statistics for sending/receiving messages at scale with both libraries.

Turns out it will take some work to make js-waku work with the Logos simulation test (ref: https://discord.com/channels/973324189794697286/1047940460791996446/1088787192870096978)

Curious to know your thoughts if it is worth it @fryorcraken

@fryorcraken
Copy link
Collaborator

Yes this is a fair idea. But not for 2023 https://notes.status.im/Uz9HeCwZTDSYyOq36Q54cA?edit

I'd suggest to open a different issue and park it.

@danisharora099
Copy link
Collaborator

Yes this is a fair idea. But not for 2023 https://notes.status.im/Uz9HeCwZTDSYyOq36Q54cA?edit

I'd suggest to open a different issue and park it.

Sounds good :)

Future work being tracked here: #1294

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
track:restricted-run Restricted run track (Secure Messaging/Waku Product), e.g. filter, WebRTC
Projects
Archived in project
Development

No branches or pull requests

4 participants