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

feat: Support proto2 required messages #1117

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

tresabhi
Copy link

@tresabhi tresabhi commented Oct 3, 2024

Eliminate x | undefined when required is passed (done)

syntax = "proto2";

message Test {
  required Child child_a = 1;
  required uint32 child_b = 2;
  optional uint32 child_c = 3;
}

message Child {
  required string name = 1;
}

...now gets transpiled into:

export interface Test {
- child_a: Child | undefined;
+ child_a: Child;
  child_b: number;
- child_c: number;
+ child_c: number | undefined;
}

export interface Child {
  value: string;
}

Update createBaseX (done)

  1. Required messages will be created recursively by calling their respective createBaseX functions
  2. Required primitives will be declared as normal
  3. Optional primitives and messages will just be undefined
function createBaseTest(): Test {
- return { child_a: undefined, child_b: 0, child_c: 0 };
+ return { child_a: createBaseChild(), child_b: 0, child_c: undefined };
}

Make fromJSON throw an error for fields with required

  1. Add assertSet which throws a TypeError if a required field is not passed
  2. Optional fields just get set to undefined

Assert set:

function assertSet<T>(field: string, value: T | undefined): T {
  if (!isSet(value)) {
    throw new Error(`Required field ${field} is not set`);
  }

  return value as T;
}

Changes to from JSON:

fromJSON(object: any): Test {
  return {
-   child_a: isSet(object.child_a) ? Child.fromJSON(object.child_a) : undefined,
-   child_b: isSet(object.child_b) ? globalThis.Number(object.child_b) : 0,
+   child_a: Child.fromJSON(assertSet('Test.child_a', object.child_a)),
+   child_b: globalThis.Number(assertSet('Test.child_b', object.child_b)),
    child_c: isSet(object.child_c) ? globalThis.Number(object.child_c) : 0,
}

Update documentation (done)

@tresabhi tresabhi changed the title Abide proto2's required label proto2 compatibility Oct 3, 2024
@tresabhi

This comment was marked as outdated.

@stephenh
Copy link
Owner

stephenh commented Oct 5, 2024

Hi @tresabhi , thanks for the PR!

I've only scanned it, but it lgtm so far...

Can you update the integration/proto2/proto2-optionals-test.ts test with some minimal changes (doesn't have to be very fancy) that would have not compiled/not worked before your changes, but that compile/now work with your changes?

Just to provide us with some regression tests going forward. Thanks!

@stephenh stephenh changed the title proto2 compatibility feat: Support proto2 required messages Oct 5, 2024
@tresabhi
Copy link
Author

tresabhi commented Oct 5, 2024

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

@tresabhi tresabhi marked this pull request as draft October 5, 2024 18:52
@tresabhi tresabhi marked this pull request as ready for review October 5, 2024 19:42
@tresabhi tresabhi marked this pull request as draft October 5, 2024 20:00
@tresabhi tresabhi marked this pull request as ready for review October 5, 2024 20:05
@tresabhi
Copy link
Author

tresabhi commented Oct 6, 2024

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.

@stephenh
Copy link
Owner

stephenh commented Oct 6, 2024

@tresabhi looks great so far! Looks like maybe more of the integration/* test need updating, if you could take a look (sorry, GitHub makes me hit Run & Approve on the PR for you to see the test runs 🤷 ). Thanks!

@tresabhi
Copy link
Author

Stephen, just fyi, I'm working on it; it's just been slow because I have been a bit busy irl.

@stephenh
Copy link
Owner

No problem, thanks for the update!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants