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

Add Relay viewer field #49

Merged
merged 3 commits into from
Jun 1, 2016
Merged

Add Relay viewer field #49

merged 3 commits into from
Jun 1, 2016

Conversation

calebmer
Copy link
Collaborator

@calebmer calebmer commented May 11, 2016

Adds a Relay viewer field to the root query type and to all of the mutation payloads. @hardchor, @msimulcik is this what you’re looking for?

Closes #38 and maybe #43.

This was referenced May 11, 2016
@msimulcik
Copy link

Great work, it's almost perfect! However, Viewer has to implement Node interface to be used with relay mutations. The open question here is what should be the id field of viewer node. I was thinking about id of current postgresql role.

Another thing, that is much easier to add, is missing edge field in mutation payload. The edge type definition is already there, it just needs to be attached to mutation payload.

For example, currently InsertTodoPayload contains:

todo: Todo
clientMutationId: String
viewer: Viewer!

The desired structure is:

todo: Todo
todoEdge: TodoEdge
clientMutationId: String
viewer: Viewer!

@calebmer
Copy link
Collaborator Author

So Relay wants the edge with the cursor?

@calebmer
Copy link
Collaborator Author

Also, I really can't believe Relay wants Viewer to be a Node, if so this might be the wrong implementation. Do you have a source for that?

@msimulcik
Copy link

Answers for both questions can be found in Relay docs for RANGE_ADD mutation

So Relay wants the edge with the cursor?

This is required for edgeName field in mutation configuration (see linked docs).

Also, I really can't believe Relay wants Viewer to be a Node, if so this might be the wrong implementation. Do you have a source for that?

Relay documentation doesn't directly mention Viewer field much, so this requirement is rather implicit. Take a look on the ParentID field in docs. Description says: "The DataID of the parent node that contains the connection". In our case the parent of all connections is Viewer so it has to be node and it has to have ID.

Reindex docs on mutations also contain good examples and explanations.

@ghost
Copy link

ghost commented May 18, 2016

@calebmer Yes, it does need to be a node, and it does need to implement the NodeInterface.

Re: the discussion around the viewer ID. In all my previous apps and others I've seen, this was always a singleton, so the ID doesn't really matter. The viewer node is simply a static entry point. I've implemented this as id: viewer, facebook use id: me (afaik).

@calebmer
Copy link
Collaborator Author

Ok, so I'd also need the node field to return viewers?

@ghost
Copy link

ghost commented May 18, 2016

That's correct. The viewer field type itself is really simple, but it does require a bit of wiring up, unfortunately.

@calebmer
Copy link
Collaborator Author

After some work with Relay I better understand the need for this. With that knowledge, do you think viewer is the right name? What do you think about making it called root or query?

Then the name viewer can then be reserved for a procedure returning a User type or something.

@ferdinandsalis
Copy link
Contributor

ferdinandsalis commented May 27, 2016

@calebmer see this issue facebook/relay#820. However viewer is used in all public examples.

@ghost
Copy link

ghost commented May 27, 2016

I agree that viewer is quite frontend-specific (e.g. relay), but query would be confusing (query { query { ... } }). Also, it kinda of is a "view" of your data structure *

* lame excuse ;)

@calebmer
Copy link
Collaborator Author

Ok, viewer is general enough. We could recommend me or game etc. for something more application specific. Also people instantly understand what viewer is.

I'll implement Node for viewer then merge this in so people can start using it. I'll implement @msimulcik's advised mutation fields in another PR.

@calebmer
Copy link
Collaborator Author

Ok, implemented node for viewer field. If you guys think this is ready to merge in it’s most basic form let me know!

Look at this and this integration test files to see the viewer field used.

@ferdinandsalis
Copy link
Contributor

ferdinandsalis commented May 29, 2016

If I am not mistaken the viewer type is missing the id which is required to configure mutations (e.g. RANGE_ADD see https://facebook.github.io/relay/docs/guides-mutations.html#range-add). The commonly used id in examples is just a plain string of me. Also see this comment #49 (comment)

I am confused … since your fixtures actually include the id and the node interface ☺️. I was on an old commit so it appears 😅. Sorry!

@msimulcik
Copy link

Looks great! I'm gonna try it with Relay today and let you know.

@calebmer calebmer merged commit 539c29d into master Jun 1, 2016
@calebmer calebmer deleted the feat/relay-viewer-field branch June 1, 2016 12:05
@calebmer
Copy link
Collaborator Author

calebmer commented Jun 1, 2016

Released in 1.5.0 🙌

@ryanblakeley
Copy link

ryanblakeley commented Oct 13, 2016

@calebmer
After some work with Relay I better understand the need for this. With that knowledge, do you think viewer is the right name? What do you think about making it called root or query?

Then the name viewer can then be reserved for a procedure returning a User type or something.

Is there a plan for viewer in #145? What if this was called root and then viewer resolved a user node for the person who is logged in?

@calebmer
Copy link
Collaborator Author

@rojobuffalo:

In #145 I did end up renaming viewer to query as I think many users would like to define their own custom viewer field in the form of a procedure 😊

the functionality is exactly the same and you can always alias query to viewer if you prefer that 👍

@valoricDe
Copy link
Contributor

Aliasing is done by query { viewer: query { ... } } right?

@calebmer
Copy link
Collaborator Author

calebmer commented Oct 28, 2016

@Valorize correct 👍

Out of curiosity, why are you interested in aliasing the field to viewer?

@valoricDe
Copy link
Contributor

@calebmer Just saw that you mentioned it's possible. So I wanted to be sure how to do it. I guess it's best to use as less aliases as possible.

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.

None yet

5 participants