-
Notifications
You must be signed in to change notification settings - Fork 349
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
feat!: Support proto2
required messages
#1117
base: main
Are you sure you want to change the base?
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Hi @tresabhi , thanks for the PR! I've only scanned it, but it lgtm so far... Can you update the Just to provide us with some regression tests going forward. Thanks! |
This test has helped me find a few more edge cases. I'm on it already before I update the test. Edit: I have resolved all issues that I found Edit 2: nope, found some issues using it in my own project, back to the drawing board Edit 3: ok everything's cool now |
I implemented this experimental version onto my website: https://blitzkit.app/, which serves as a pretty good benchmark for the tech and works without a single hitch. The proto files aren't anything to be scoffed at: https://github.com/tresabhi/blitzkit/tree/dev/packages/core/src/protos. |
@tresabhi looks great so far! Looks like maybe more of the |
Stephen, just fyi, I'm working on it; it's just been slow because I have been a bit busy irl. |
No problem, thanks for the update! |
proto2
required messagesproto2
required messages
@stephenh I think this PR is ready. I just for the love of all things good cannot figure out why |
Would love to use this!!! Please merge as soon as you can |
Eliminate
x | undefined
whenrequired
is passed (done)...now gets transpiled into:
Update
createBaseX
(done)createBaseX
functionsundefined
Make
fromJSON
throw an error for fields withrequired
assertSet
which throws aTypeError
if a required field is not passedAssert set:
Changes to from JSON:
Update documentation (done)