Use `@bufbuild` #337

felicio · 2023-02-03T22:37:30Z

Resolves #325

felicio · 2023-02-06T15:44:17Z

packages/status-js/buf.gen.yaml

@@ -1,6 +1,6 @@
-version: v1beta1
-
+version: v1


Uses stable version (see https://docs.buf.build/generate/usage).

felicio · 2023-02-06T15:49:50Z

packages/status-js/src/protos/buf.yaml

+version: v1
+breaking:
+  use:
+    - FILE
 lint:
+  use:
+    - DEFAULT
  except:
    - ENUM_ZERO_VALUE_SUFFIX
    - ENUM_VALUE_PREFIX


Related buf commands work relative to the file's location, recursively. So instead of excluding all found files, for example from node_modules, this config is placed next our .proto definitions to match only those.

felicio · 2023-02-06T16:02:53Z

packages/status-js/src/client/community/community.ts

 import type { Client } from '../client'
+import type { PlainMessage } from '@bufbuild/protobuf'


⚠️ Uses PlainMessage utility type, because classes are passed around and @bufbuild/protoc-gen-es does not generate interfaces like protons does.

See "OK, so why not generate both classes and interfaces?".

I must admit that I like how protons uses same name for both the interface and encoding function under a namespace, and thus allowing one import, by relying on type inference:

the compiler infers whether it refers to the type alias or the namespace based on the usage. – https://stackoverflow.com/questions/62604227/namespace-with-the-same-name-as-type-alias-in-the-same-file

I wonder why they didn't consider ES modules as a valid approach...

They,

say "ECMAScript module support" at https://www.npmjs.com/package/@bufbuild/protobuf

have module set at https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/package.json#L19

build it at https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/package.json#L16

and export as such at https://github.com/bufbuild/protobuf-es/blob/main/packages/protobuf/package.json#L22

What am I missing 🤔?

Also, is there anything else before the PR can be approved?

felicio · 2023-02-06T16:04:42Z

packages/status-js/src/client/community/community.ts

      clock: this.setClock(this.#clock),
      chatId,
      communityId: hexToBytes(this.id),
      ensName: '',
-    })
+    }).toBinary()


⚠️ Payloads are encoded with new and toBinary() instead of encode().

felicio · 2023-02-06T16:05:46Z

packages/status-js/src/client/community/handle-waku-message.ts

        // decode
-        const decodedPayload = CommunityDescription.decode(messageToDecode)
+        const decodedPayload = CommunityDescription.fromBinary(messageToDecode)


⚠️ Payloads are decoded with fromBinary() instead of decode().

felicio · 2023-02-06T16:12:39Z

packages/status-js/src/protos/chat-message_pb.ts

+   *
+   * @generated from field: ChatMessage.ContentType content_type = 8;
+   */
+  contentType = ChatMessage_ContentType.UNKNOWN_CONTENT_TYPE;


⚠️ Uses enum values (i.e. number).

See "Why not use string unions for Protobuf enumerations instead of TypeScript enum?".

prichodko · 2023-02-07T10:59:28Z

What does it use for a runtime?

prichodko · 2023-02-07T11:04:32Z

I would love to know the size of generated files. I can help with this if necessary.

prichodko · 2023-02-07T11:05:00Z

Thanks for doing this. Feels good to get rid of this debt.

felicio · 2023-02-07T11:15:15Z

I would love to know the size of generated files. I can help with this if necessary.

Does

Use single protobuf generator and runtime #325 (comment)

answer the bundle size, and

https://github.com/status-im/status-web/blob/5b48b614e86c7917631fab43e1a7b9b3b8865854/packages/status-js/src/proto/communities/v1/emoji_reaction.ts (324 lines; protobufjs)
https://github.com/status-im/status-web/blob/5b48b614e86c7917631fab43e1a7b9b3b8865854/packages/status-js/src/protos/emoji-reaction.ts (110 lines, protons)
https://github.com/status-im/status-web/blob/d034b415fe62c234d0f0cbe6eac91eaabaa36dd3/packages/status-js/src/protos/emoji-reaction_pb.ts (147 lines, bufbuild; lots of comments and new lines)

the files size?

Or did you have something else in mind?

felicio · 2023-02-07T11:16:52Z

What does it use for a runtime?

@bufbuild/protobuf

prichodko · 2023-02-08T10:05:17Z

In the end, what matters is not the number of lines, but the final size after all dependencies are bundled and optimized. The comment in #325 is a good indicator. 👍

fryorcraken · 2023-02-10T12:55:05Z

I see some size gain from this change. Anything else?

felicio · 2023-02-10T17:39:38Z

@fryorcraken

I see some size gain from this change. Anything else?

For this repo:

single runtime (technical debt)
supports oneof in ChatMessage
handles CommunityDescription correctly (Use single protobuf generator and runtime #325 (comment))
- @bufbuild just worked, didn't spend time debugging protons but wouldn't mind switching if resolved in the future
  - possibly, unsupported nested maps
  - also extending https://github.com/ipfs/protons/tree/master/packages/protons-benchmark for @bufbuild might be interesting
    - actually, chore: benchmark Protobuf-ES ipfs/protons#89
compatibility (Use single protobuf generator and runtime #325 (comment)) and alternatives comparison (https://buf.build/blog/protobuf-es-the-protocol-buffers-typescript-javascript-runtime-we-all-deserve/#alternatives) were just reassuring (didn't dig deeper)

fryorcraken · 2023-02-17T03:03:22Z

thanks @felicio . All great reason.

Could you help me understand this technical debt?

single runtime (technical debt)

Are you saying bufbuild uses async? However, everything is done in JavaScript runtime, right? so how does it help?

felicio · 2023-02-17T04:52:39Z

@fryorcraken

Could you help me understand this time?

I see how it could be confusing now. It just that we've experimented with different protobuf runtime (i.e. protons-runtime) but still left protobufjs (at diff version) in the project too handling CommunityDescription, for instance.

With this PR, I'm proposing to drop the two for just one – @bufbuild/protobuf.

prichodko

👍

prichodko · 2023-02-21T11:36:05Z

packages/status-js/package.json

-    "long": "^5.2.0",
-    "protobufjs": "^6.11.3",
-    "protons-runtime": "^1.0.4"
+    "long": "^5.2.0"


Do we still need Long package?

Removed at 2bdcc54

* remove old `communities.proto` * fix `protos` npm script * apply `protos` changes overwriting `Record<>` patches * update proto files * regenerate files * update `protons*` packages * regen files * use `protons` generate communities * add `@bufbuild*` packages * update `buf` yaml files * generate `@bufbuild` files * use `@bufbuild` files * format * remove `protobufjs` * remove `protons` * resolve build errors * fix image content type

felicio added 17 commits February 3, 2023 18:40

remove old communities.proto

6edbd85

fix protos npm script

68da0b8

apply protos changes overwriting Record<> patches

8b2d2f3

update proto files

ab24010

regenerate files

b762f70

update protons* packages

a1d4f0d

regen files

9d24b86

use protons generate communities

66a5d9e

add @bufbuild* packages

af66b31

update buf yaml files

535f5f7

generate @bufbuild files

4b98128

use @bufbuild files

8beb477

format

66d2a27

remove protobufjs

bea7e5b

remove protons

b5d4b8d

resolve build errors

e9ccce5

fix image content type

d034b41

felicio commented Feb 6, 2023

View reviewed changes

felicio changed the title ~~WIP: Use @bufbuild~~ Use @bufbuild Feb 6, 2023

felicio mentioned this pull request Feb 10, 2023

Add request client #342

Merged

felicio requested a review from prichodko February 14, 2023 08:16

fryorcraken mentioned this pull request Feb 14, 2023

Use proto library that provides tree-shakeable code waku-org/js-waku#335

Closed

2 tasks

prichodko merged commit effd3ea into status-im:main Feb 21, 2023

prichodko reviewed Feb 21, 2023

View reviewed changes

felicio self-assigned this May 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `@bufbuild` #337

Use `@bufbuild` #337

felicio commented Feb 3, 2023 •

edited

Loading

felicio Feb 6, 2023

felicio Feb 6, 2023

felicio Feb 6, 2023 •

edited

Loading

felicio Feb 7, 2023

prichodko Feb 8, 2023

felicio Feb 10, 2023

felicio Feb 6, 2023

felicio Feb 6, 2023

felicio Feb 6, 2023 •

edited

Loading

prichodko commented Feb 7, 2023

prichodko commented Feb 7, 2023

prichodko commented Feb 7, 2023

felicio commented Feb 7, 2023

felicio commented Feb 7, 2023

prichodko commented Feb 8, 2023

fryorcraken commented Feb 10, 2023

felicio commented Feb 10, 2023 •

edited

Loading

fryorcraken commented Feb 17, 2023 •

edited

Loading

felicio commented Feb 17, 2023 •

edited

Loading

prichodko left a comment

prichodko Feb 21, 2023

felicio Feb 21, 2023

		import type { Client } from '../client'
		import type { PlainMessage } from '@bufbuild/protobuf'

Use @bufbuild #337

Use @bufbuild #337

Conversation

felicio commented Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felicio Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felicio Feb 6, 2023 • edited Loading

Choose a reason for hiding this comment

prichodko commented Feb 7, 2023

prichodko commented Feb 7, 2023

prichodko commented Feb 7, 2023

felicio commented Feb 7, 2023

felicio commented Feb 7, 2023

prichodko commented Feb 8, 2023

fryorcraken commented Feb 10, 2023

felicio commented Feb 10, 2023 • edited Loading

fryorcraken commented Feb 17, 2023 • edited Loading

felicio commented Feb 17, 2023 • edited Loading

prichodko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Use `@bufbuild` #337

Use `@bufbuild` #337

felicio commented Feb 3, 2023 •

edited

Loading

felicio Feb 6, 2023 •

edited

Loading

felicio Feb 6, 2023 •

edited

Loading

felicio commented Feb 10, 2023 •

edited

Loading

fryorcraken commented Feb 17, 2023 •

edited

Loading

felicio commented Feb 17, 2023 •

edited

Loading