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

Make mutations in Relay easier by including related tables #115

Closed

Conversation

tim-field
Copy link
Contributor

@tim-field tim-field commented Sep 18, 2016

This fixes the issue described in #114

With this update in place I'm able to instruct Relay to correctly update its local store by following along with the core Relay docs.

  getFatQuery() {
    return Relay.QL`
      fragment on InsertCommentPayload {
        commentEdge
        postByPostId { commentNodesByPostId }
      }
    `
  }

  getConfigs() {
    return [{
      type: 'RANGE_ADD',
      parentName: 'postByPostId',
      parentID: this.props.post.id,
      connectionName: 'commentNodesByPosttId',
      edgeName: 'commentEdge',
      rangeBehaviors: {
        '': 'prepend',
      }
    }]
  }

I think something like this is really important in order to use this library with Relay as the only other way to get around this seems to be to add force fetch calls, and after all Relay exists to minimise unnecessary fetching :)

I suspect that I'm not doing this the most efficient way, please review and suggest any improvements.

Copy link
Collaborator

@calebmer calebmer left a comment

Choose a reason for hiding this comment

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

Tests ✏️


// Add foreign key field references.
...fromPairs(
table.getForeignKeys().map(foreignKey => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add a test for this? Thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I haven't been able to get the test suite to run locally yet :/

FYI This is just using existing functionality from createTableType.js

Copy link
Contributor

Choose a reason for hiding this comment

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

@tim-field do you need any help in setting up the tests?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ferdinandsalis Yes Please! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@tim-field you can get a hold of me on gitter most of the time. Just oben a direct message with me. Hope I can help.

@calebmer
Copy link
Collaborator

Thanks so much for the PR! Looks good, just make sure to clean up the lint errors which are causing CI to fail and add some tests.

I’m going to need to think about this one. I recognize the present benefit, but it would be adding technical debt which may not be necessary in the future of Relay.

@tim-field
Copy link
Contributor Author

tim-field commented Sep 20, 2016

I hear you. But this is quite a small change, and I feel that much more technical debt is being introduced into Relay applications built using postgraphql to work around this limitation. Everyone who uses postgraphql is going to come across this issue at some point if they are using Relay.

@calebmer
Copy link
Collaborator

Yeah, I think I’m fine with this. Just needs tests. Let’s explore other naming options for a bit though. Currently you’d have a mutation that could look something like this:

type InsertPostPayload {
  post: Post
  personByAuthorId: Person
  …
}

What other names might we be able to use in order to make the meaning more explicit? How about personByPostAuthorId instead of personByAuthorId. That effectively “namespaces” the field which makes me feel a little bit better.

@ferdinandsalis
Copy link
Contributor

@tim-field when I try to update the graphql schema (with babel-relay-plugin) of my current project, I get the following error with this pull request,

Schema must be an instance of GraphQLSchema. Also ensure that there are not multiple versions of GraphQL installed in your node_modules directory

I dont have multiple versions of GraphQL and it does work with the master branch.

@tim-field
Copy link
Contributor Author

@ferdinandsalis that is strange, I've been running this fork for a while in my project without issue. The pull request is quite a small change, out of interest do you get the same error after commenting out the ...fromPairs spread starting here?

d40b134#diff-994c91cd9decabe47b58fe286491fde6R91

@ferdinandsalis
Copy link
Contributor

@tim-field It is strange. I will check tomorrow and report back to you.

@ferdinandsalis
Copy link
Contributor

ferdinandsalis commented Sep 26, 2016

@tim-field I am sorry. After all its the duplicate graphql instances that causes the problem, since I use babel-relay-plugin. I had to use a locally built postgraphql (with npm link) because I can not install from github — it does not include the babel compiled dist directory. Furthermore npm can not dedupe node modules in a symlinked package (npm/npm#7742). So it seems there is no way for me test this with my project. I desperatly need it though 😬 Maybe I am missing something. Is there still a way to do it?

Concerning the tests what is it you need help with? Lets get this merged 😊

@tim-field
Copy link
Contributor Author

@ferdinandsalis so the way I've been using this is I installed postgraphql as a dependency to my project ( from npm as per usual ) then in another directory I checked out this fork, and ran npm run build, then I manually replaced node_modules/postgraphql with a symlink to the forked directory. This enabled me to use the fork without issue in my project.

@ferdinandsalis
Copy link
Contributor

@tim-field this is more or less what npm link does. Also I did try it with your method. No luck, as expected. Its no problem to run the server but using babel-relay-plugin to update the schema renders to stated problem.

@tim-field
Copy link
Contributor Author

@ferdinandsalis hmm ok, I'm using https://github.com/graphcool/babel-plugin-react-relay in my project, maybe try that ? I've found it much easier to work with than babel-relay-plugin

@ferdinandsalis
Copy link
Contributor

ferdinandsalis commented Sep 28, 2016

@tim-field so I got it to work 😊 It would also make sense to add the related tables it to the other mutation payloads (delete and update).

Did you get the tests to work?

@calebmer calebmer mentioned this pull request Oct 10, 2016
4 tasks
@calebmer
Copy link
Collaborator

This will be included in PostGraphQL 2 for create mutations, update mutations, delete mutations, and procedure mutations. Thanks so much @tim-field for convincing me this is a good idea (and easy to implement), and thanks @ferdinandsalis for helping out 👍

To start using this feature immediately try out the PostGraphQL 2 beta (#145)

npm install -g postgraphql@next
postgraphql

@calebmer calebmer closed this Oct 10, 2016
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.

3 participants