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

Return specific Type on specific Mutation #5893

Merged
merged 3 commits into from Aug 12, 2019

Conversation

@Moumouls
Copy link
Member

commented Aug 7, 2019

Return specific Type on specific Mutation

  • Return Type on Create
  • Return Type on Update
  • Return Type on Delete
  • Optimization
  • Return Me on SignUp

Create

mutation {
  objects {
    createExample(
      fields: {
        name: "Hello"
        price: 10
        tags: ["hello","world"]
        description: "A good hello world"
      }
    ) {
      objectId
      description
      tags
    }
  }
}

Update

mutation {
  objects {
    updateExample(
      objectId: "jsh7cs6j"
      fields: {
        description: "A good hello world modified"
      }
    ) {
      objectId
      description
      tags
      name
    }
  }
}

Delete

mutation {
  objects {
    deleteExample(
      objectId: "jsh7cs6j"
    ) {
      objectId
      description
      tags
      name
    }
  }
}

Change login Args

Convert the login args into an input

Return type Me on Signup

I made a shortcut, it probably needs to be improved.

#5863

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Moumouls I like the implementation. I think we can try to optimize the sign up process and also try to only do the additional gets when necessary. I can send you some tips on how to do it. But, before that, I wanted to learn more about the benefits in returning back all these fields. It is little bit hard for me to understand the pros of having it returning in the mutation vs the cons of having one more database query per mutation. It is even harder for me to understand the benefits when talking about the delete mutation. Could you explain?

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Returning fields on Mutation allow front-developers to update easly the front Apollo Client cache for example (really useful in the React Apollo Client), or in one time request to just update one field and request other fields in one time request for UI purpose.
Auto cache Apollo Example

On delete, returning fields like objectId allow in first time to update the front-cache (front-store) and then other fields can be used for UI purpose (dialog, snackbar confirmation).

Cons: yes it's one more REST request to the backend, but a get rest request is fully indexed on the objectId and it's a super light/super fast call non cpu intensive.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

It also seems that for efficient caching in front-end applications using Apollo Client currently the Parse implementation is not compatible by default. If we keep the objectId as an identifier, we must inform users (in the case of Apollo Client use) to change the inMemoryCache policy.

// Parse compatible cache policy
const cache = new InMemoryCache({
  dataIdFromObject: object => object.objectId || null
});
@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Moumouls I got it. So let's move forward with this. Let's discuss the objectId thing on a separate thread?

@davimacedo
Copy link
Member

left a comment

@Moumouls It looks good to me. I am only suggesting few optimizations to avoid the down side of having an additional read operation in the database for each mutation. But let me know your thoughts. Maybe we can optimize it later.

src/GraphQL/loaders/parseClassMutations.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassMutations.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/parseClassMutations.js Outdated Show resolved Hide resolved
src/GraphQL/loaders/usersMutations.js Outdated Show resolved Hide resolved
@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@Moumouls I got it. So let's move forward with this. Let's discuss the objectId thing on a separate thread?

Yes we need more reflexion and feedbacks for this thing

We can't optimize

After a short thinking about the full Rest Get on Mutation we do not have really the choice, because if a developer write some custom logic in a beforeSave hook, and change on the fly some inputed fields, the returned GraphQL Object could be wrong.
Here optimization on Get will lead to inconsistency data...

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

That's not true. When some change is done in a trigger, the changes are returned by the rest operation.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@davimacedo oohh sorry
Does the REST API for creating and updating return all modified fields (after a beforeSave)?
After reading the documentation, the Create Rest function seems to retrieve only the objectId and createdAt:
https://docs.parseplatform.org/rest/guide/#creating-objects

only updatedAt for the update: https://docs.parseplatform.org/rest/guide/#updating-objects

Are the docs correct?

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

I think the docs were written before the triggers were introduced. They are actually wrong. The REST API returns all modified fields for both create and update operations after the afterTrigger runs. I reviewed a user's issue another day regarding this and I discovered that. The docs also confused me in that time. I will create an issue in the docs repo.

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

BTW this was the PR that I learned about it :) It was added support to change the returned fields in the after save trigger.
50f1e8e

@davimacedo

This comment has been minimized.

Copy link
Member

commented Aug 8, 2019

Now thinking again about the triggers that can change the original input, I see this PR's returning all fields even more useful for the developers :)

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

@davimacedo okay so lets finish the work here for a full optimized return on Mutations
I will take your suggestions into account

@codecov

This comment has been minimized.

Copy link

commented Aug 9, 2019

Codecov Report

Merging #5893 into master will decrease coverage by <.01%.
The diff coverage is 97.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5893      +/-   ##
==========================================
- Coverage   93.71%   93.71%   -0.01%     
==========================================
  Files         153      153              
  Lines       10727    10765      +38     
==========================================
+ Hits        10053    10088      +35     
- Misses        674      677       +3
Impacted Files Coverage Δ
src/GraphQL/loaders/parseClassTypes.js 86.63% <100%> (+0.17%) ⬆️
src/GraphQL/loaders/usersMutations.js 90.32% <100%> (ø) ⬆️
src/GraphQL/loaders/parseClassMutations.js 98.83% <97.36%> (-1.17%) ⬇️
src/Adapters/Auth/apple.js 87.09% <0%> (-2.56%) ⬇️
src/RestWrite.js 93.56% <0%> (-0.17%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0e9462b...aadc9bd. Read the comment docs.

@Moumouls

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Everything seems good for me, if it seems good for you (@davimacedo ) too, tell me and then i will squash my commits !

@davimacedo
Copy link
Member

left a comment

Nice job! LGTM!

@davimacedo davimacedo merged commit 9031961 into parse-community:master Aug 12, 2019

2 checks passed

Danger All good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.