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.

Sign up for GitHub

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

Jump to bottom

feat(grpc): additional scalar types #2002

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

feat(grpc): additional scalar types #2002

wants to merge 15 commits into from
+2,828 −206

Conversation

meskill
Copy link
Contributor

@meskill meskill commented May 21, 2024

Summary:
Briefly describe the changes made in this PR.

Issue Reference(s):
Fixes #1896

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label May 21, 2024
@codecov Codecov
Copy link

codecov bot commented May 21, 2024 •
edited

Codecov Report

Attention: Patch coverage is 98.13953% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 82.91%. Comparing base (55fb2e3) to head (98c6112).
Report is 1 commits behind head on main.

Files Patch % Lines
tailcall-typedefs/src/gen_gql_schema.rs 0.00% 3 Missing ⚠️
tailcall-typedefs/src/main.rs 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2002      +/-   ##
==========================================
+ Coverage   82.70%   82.91%   +0.21%     
==========================================
  Files         178      184       +6     
  Lines       18043    18236     +193     
==========================================
+ Hits        14922    15121     +199     
+ Misses       3121     3115       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

TheWizardTower and others added 5 commits May 22, 2024 13:55
@TheWizardTower @tusharmath
@meskill @renovate @mehulmathur16
chore: Clean up merge logic for vectors of key-value structs. (#1977)
75b31e4 
Co-authored-by: Tushar Mathur <tusharmath@gmail.com>
Co-authored-by: Kiryl Mialeshka <8974488+meskill@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: Mehul Mathur <mathur.mehul01@gmail.com>
@amitksingh1490 @tusharmath
fix: errors on compile time on mac (#2005)
8f9a91a
@tusharmath
chore: update prettier command
043744f
@tusharmath
chore: update prettier config reading logic
8dc11b5
@tusharmath
Merge branch 'main' into feat/grpc-scalar-types
a6a393a
@github-actions GitHub Actions
Copy link

Action required: PR inactive for 2 days.
Status update or closure in 5 days.

@github-actions github-actions bot added the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 25, 2024
@github-actions github-actions bot removed the state: inactive No current action needed/possible; issue fixed, out of scope, or superseded. label May 27, 2024
tusharmath
tusharmath reviewed May 31, 2024
View reviewed changes
src/core/generator/from_proto.rs
"bool" => "Boolean",
"string" | "bytes" => "String",
"string" => "String",
"bytes" => "Bytes",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think for numbers we should use the following:

Rust Types Protobuf Primitive GraphQL Scalar
i8, i16, i32 int32 Int
u8, u16, u32 uint32 Int
i64 int64 BigInt
u64 uint64 BigInt
f32 float Float
f64 double Float
bool bool Boolean
char string String
usize, isize int64 or uint64 BigInt

Generally the consumer of GraphQL is going to be a JS developer who doesn't work with so many types usually. It would be easier on them to reduce it to some simple one that are compatible with something that exists in JS. This helps build a better intuition for a developer who is consuming a GraphQL API. It's hard for a JS engineer to make sense of what should UInt32 map to natively.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that wrapping so many different types into single one is a robust solution since the conversion from BigInt to any other int type will lose the precision or fail. And for grpc case, API expects a value with defined restrictions that could be expressed with respective scalar type. In that case scalar acts as validation step before sending invalid data to the upstream or generating conversion error. Also it acts as documentation on type level that helps to understand API expectations without a need to search for API docs or explore validation errors.

I feel like having such validation as scalar type (if we improve error message) will be easier to understand compared to conversion error or API error that could appear in such cases.

tusharmath
tusharmath requested changes Jun 1, 2024
View reviewed changes
Copy link
Contributor

@tusharmath tusharmath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tusharmath tusharmath tusharmath requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat(grpc): Implement additional Scalar Types
4 participants
@meskill @tusharmath @TheWizardTower @amitksingh1490