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

refactor(experimental): graphql: slot-based nested queries #2643

Closed
wants to merge 3 commits into from
Closed

refactor(experimental): graphql: slot-based nested queries #2643

wants to merge 3 commits into from

Conversation

buffalojoec
Copy link
Collaborator

@buffalojoec buffalojoec commented May 6, 2024 •
edited
Loading

As the linked issue describes, the GraphQL resolver sports full support for
nested Account queries wherever a field has type Address. Instead of
defining that field to simply be of type Address, it's defined with type Account,
which enables nested account queries on that field based on the address
returned.

In this PR, we're taking every field with type Slot and replacing it with Block,
to enable nested block queries in the same fashion.

Closes #1822.

@changeset-bot changeset-bot
Copy link

changeset-bot bot commented May 6, 2024 •
edited
Loading

⚠️ No Changeset found

Latest commit: d524067

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@buffalojoec buffalojoec mentioned this pull request May 6, 2024
Merged
@buffalojoec Graphite App
Copy link
Collaborator Author

buffalojoec commented May 6, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @buffalojoec and the rest of your teammates on Graphite Graphite

@buffalojoec buffalojoec mentioned this pull request May 6, 2024
Merged
@buffalojoec buffalojoec force-pushed the 05-05-refactor_experimental_graphql_group_schema_type_resolvers branch from 5f9f41e to e14dfd1 Compare May 6, 2024 12:51
@buffalojoec buffalojoec force-pushed the 05-05-refactor_experimental_graphql_slot-based_nested_queries branch from 2fe1d96 to d400e65 Compare May 6, 2024 12:52
@buffalojoec buffalojoec marked this pull request as ready for review May 6, 2024 12:52
steveluscher
steveluscher requested changes May 10, 2024
View reviewed changes
Copy link
Collaborator

@steveluscher steveluscher left a comment

Choose a reason for hiding this comment

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

I think if we're going to drink deep from this cup, we should probably rename everything to reflect the return type.

packages/rpc-graphql/src/loaders/__tests__/transaction-loader-test.ts
@@ -55,7 +61,9 @@ describe('account loader', () => {
const source = /* GraphQL */ `
query testQuery($signature: Signature!) {
transaction(signature: $signature) {
slot
slot {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this need to be renamed to block then, to reflect the new return type?

packages/rpc-graphql/src/resolvers/block.ts Show resolved Hide resolved
packages/rpc-graphql/src/schema/type-defs/account.ts
Comment on lines +60 to +296
deactivationSlot: Block
lastExtendedSlot: Block
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still think these should be renamed to reflect the new return type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I truly waffled on this a bit actually. I just wanted to be careful about straying too far from the defined data, such as an account whose fields are "something-slot" and now they have to call it "block".

I couldn't decide if this was acceptable, really.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we leave it this way, it's going to lead to a lot of aggravating code like:

if (data.slot.slot < 123n) {
    // ...
}

@buffalojoec
Copy link
Collaborator Author

I think if we're going to drink deep from this cup, we should probably rename everything to reflect the return type.

I'm interested to know your thoughts on whether or not we should drink from this cup some more.

@github-actions github-actions bot added the stale label May 27, 2024
@github-actions github-actions bot closed this Jun 3, 2024
@buffalojoec buffalojoec reopened this Jun 3, 2024
@buffalojoec buffalojoec added do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot and removed stale labels Jun 3, 2024
@buffalojoec buffalojoec self-assigned this Jun 3, 2024
@buffalojoec buffalojoec force-pushed the 05-05-refactor_experimental_graphql_group_schema_type_resolvers branch from e14dfd1 to 7b2bf6c Compare June 13, 2024 17:14
@buffalojoec buffalojoec force-pushed the 05-05-refactor_experimental_graphql_slot-based_nested_queries branch from d400e65 to d524067 Compare June 13, 2024 17:14
@buffalojoec buffalojoec force-pushed the 05-05-refactor_experimental_graphql_group_schema_type_resolvers branch from 7b2bf6c to a3956cb Compare June 13, 2024 17:15
Base automatically changed from 05-05-refactor_experimental_graphql_group_schema_type_resolvers to master June 13, 2024 17:16
@buffalojoec buffalojoec mentioned this pull request Jun 13, 2024
Merged
@buffalojoec
Copy link
Collaborator Author

I hate this PR. I think if people want this feature we can add it later, but it's kind of a beast.

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

@steveluscher steveluscher steveluscher requested changes

Assignees

@buffalojoec buffalojoec

Labels
do-not-close Add this tag to exempt an issue/PR from being closed by the stalebot
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GraphQL]: Add support for slot-based nested queries
2 participants
@buffalojoec @steveluscher