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: Initial relay implementation #2511

Merged
merged 74 commits into from
Jun 6, 2023
Merged

Conversation

bellini666
Copy link
Member

@bellini666 bellini666 commented Feb 4, 2023

This is my relay implementation from strawberry-django-plus, which I mentioned in this issue

There are some things that might need some discussion:

  1. I used GlobalID as the name of the scalar to avoid conflicts with the existing ID. Although it doesn't cause issues for the majority of people, there are some cases where the client expects the scalar to be named ID. It can be worked around by overriding the ID scalar to use the GlobalID, but maybe there's a better way? Note that the GlobalID object is a dataclass with some custom attributes and methods.
  2. Find a way to allow the object to define an id, which is the non global version one, while still keeping our Node.id resolver working. You will notice in my tests that I defined the Fruit id as _id for that reason. Maybe use a metaclass approach to solve this?

TODO (already done):

  • Add tests for the input_mutation
  • Write a documentation for input_mutation

Fix #1573

@bellini666 bellini666 requested review from patrick91 and a team February 4, 2023 18:11
@codecov
Copy link

codecov bot commented Feb 4, 2023

Codecov Report

Merging #2511 (95d6fa7) into main (20bf0b7) will increase coverage by 0.16%.
The diff coverage is 98.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2511      +/-   ##
==========================================
+ Coverage   96.34%   96.51%   +0.16%     
==========================================
  Files         197      203       +6     
  Lines        8328     8774     +446     
  Branches     1525     1610      +85     
==========================================
+ Hits         8024     8468     +444     
- Misses        191      194       +3     
+ Partials      113      112       -1     

@botberry
Copy link
Member

botberry commented Feb 4, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


Initial relay spec implementation. For information on how to use
it, check out the docs in here: https://strawberry.rocks/docs/guides/relay


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Thiago Bellini Ribeiro for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

Copy link

@kaos kaos left a comment

Choose a reason for hiding this comment

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

This is a massive PR, but I hope we can get through this as it looks really well worked through. 🚀

And it will be easier to circle back and fix any remaining rough edges after we've got some experience using it.

pyproject.toml Outdated Show resolved Hide resolved
_R = TypeVar("_R")
connection_typename = "arrayconnection"

NodeType = TypeVar("NodeType", bound="Node")
Copy link

Choose a reason for hiding this comment

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

This looks like a class name to me, and reading the docs example for custom connections had me really confused (wrgt how it was implemented, not how to use it).

I'm not sure how to improve this really, though. Perhaps TNode does a better job at hinting that this is a generic type var?

however, looking at some more places this is used, I think NodeType works really well, so perhaps the doc mentioned is the only confusing part.. I'll ponder on this a bit more.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm open to change it to TNode or anything else that makes more sense. I have to admit that I'm not good at naming things (strawberry-django-plus is a good example of that hahaha)

Copy link

Choose a reason for hiding this comment

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

Naming stuff is among the top 3 most difficult programming jobs! :P

I think I landed in being in favour of keeping it as NodeType, left the comment regardless to be transparent about my review process.

tests/relay/schema.py Show resolved Hide resolved
Copy link

@kaos kaos left a comment

Choose a reason for hiding this comment

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

Nice!

docs/guides/relay.md Show resolved Hide resolved
docs/guides/relay.md Outdated Show resolved Hide resolved
@bellini666
Copy link
Member Author

Just added a doc section for the input mutation and some tests for it. I think the PR is basically done, not considering any suggestions/improvements that might arrive from code reviews, of course :)

@bellini666
Copy link
Member Author

Note: I separated the specialized generic support to another PR (#2517). As soon as it gets merged I'll rebase this PR to make the changes here be only about relay itself

[relay spec](https://relay.dev/docs/guides/graphql-server-specification/).
You can read more about it in the [relay](./input-types) page.

<Note/>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<Note/>
</Note>

@patrick91
Copy link
Member

/pre-release

Copy link
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

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

👀

@patrick91
Copy link
Member

https://media.giphy.com/media/8QtP5TqscKh3O/giphy.gif

@patrick91 patrick91 merged commit 7cab6cd into strawberry-graphql:main Jun 6, 2023
@nrbnlulu
Copy link
Member

nrbnlulu commented Jun 6, 2023

Thanks @bellini666 !!!

@kaos
Copy link

kaos commented Jun 6, 2023

amazing!!

@bellini666 bellini666 deleted the relay branch June 6, 2023 18:22
@woojing
Copy link

woojing commented Jun 8, 2023

Great job! Thanks you @bellini666 and whole reviewers :)

@lostclus
Copy link

Thank you for relay implementation. But schema like

scalar GlobalID

interface Node {
  id: GlobalID!
}

is not compatible with schema that produces Graphene when using federation

interface Node {
  id: ID!
}

So that combining Strawberry subgraph with Graphene subgraph is not possible. I think using ID! instead of GlobalID! is more right because specification use ID!. https://relay.dev/docs/guides/graphql-server-specification/#schema

@patrick91
Copy link
Member

@lostclus can you open an issue for this?

we were discussing some ideas around it on discord, but I think it is worth tracking it here on GitHub too :)

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

Successfully merging this pull request may close these issues.

Relay implementation proposal
10 participants