-
Notifications
You must be signed in to change notification settings - Fork 36
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 @bufbuild
#337
Use @bufbuild
#337
Conversation
@@ -1,6 +1,6 @@ | |||
version: v1beta1 | |||
|
|||
version: v1 |
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.
Uses stable version (see https://docs.buf.build/generate/usage).
version: v1 | ||
breaking: | ||
use: | ||
- FILE | ||
lint: | ||
use: | ||
- DEFAULT | ||
except: | ||
- ENUM_ZERO_VALUE_SUFFIX | ||
- ENUM_VALUE_PREFIX |
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.
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.
import type { Client } from '../client' | ||
import type { PlainMessage } from '@bufbuild/protobuf' |
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.
PlainMessage
utility type, because classes are passed around and @bufbuild/protoc-gen-es
does not generate interfaces like protons
does.
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.
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
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.
I wonder why they didn't consider ES modules as a valid approach...
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.
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?
clock: this.setClock(this.#clock), | ||
chatId, | ||
communityId: hexToBytes(this.id), | ||
ensName: '', | ||
}) | ||
}).toBinary() |
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.
new
and toBinary()
instead of encode()
.
// decode | ||
const decodedPayload = CommunityDescription.decode(messageToDecode) | ||
const decodedPayload = CommunityDescription.fromBinary(messageToDecode) |
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.
fromBinary()
instead of decode()
.
* | ||
* @generated from field: ChatMessage.ContentType content_type = 8; | ||
*/ | ||
contentType = ChatMessage_ContentType.UNKNOWN_CONTENT_TYPE; |
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.
number
).
See "Why not use string unions for Protobuf enumerations instead of TypeScript enum?".
What does it use for a runtime? |
I would love to know the size of generated files. I can help with this if necessary. |
Thanks for doing this. Feels good to get rid of this debt. |
Does answer the bundle size, and
the files size? Or did you have something else in mind? |
|
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. 👍 |
I see some size gain from this change. Anything else? |
For this repo:
|
thanks @felicio . All great reason. Could you help me understand this technical debt?
Are you saying bufbuild uses async? However, everything is done in JavaScript runtime, right? so how does it help? |
I see how it could be confusing now. It just that we've experimented with different protobuf runtime (i.e. With this PR, I'm proposing to drop the two for just one – |
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.
👍
"long": "^5.2.0", | ||
"protobufjs": "^6.11.3", | ||
"protons-runtime": "^1.0.4" | ||
"long": "^5.2.0" |
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.
Do we still need Long
package?
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.
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
Resolves #325