Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Fix mutation promise result type and provide utility to work with the result #132

Merged
merged 1 commit into from
Nov 22, 2018

Conversation

svsool
Copy link
Contributor

@svsool svsool commented Sep 10, 2018

Hi, thanks for this library.

I think that mutation promise is wrongly typed and it should be typed as Js.Promise.t(executionResult)

That means I can't use convertJsInputToReason to parse data in Js.Promise.then_ as for instance it's done here https://github.com/tmepple/my-react-app/blob/d47b0b0f2de3d44b1b8dd06f0db2194c0ef987c3/src/components/SignupForm.re#L122 because it's not safe

This PR is intended to fix aforementioned problems

@apollo-cla
Copy link

@svsool: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@fakenickels
Copy link
Contributor

Good one!

@baransu
Copy link
Contributor

baransu commented Nov 12, 2018

What's required to merge and ship it?

@Gregoirevda
Copy link
Contributor

I reverted
shouldn't the transformation from JS Object to Variant be done when the mutation is called?
https://github.com/apollographql/reason-apollo/pull/132/files#diff-58f21bf0169e41485aa3fd2553abd3e5R41

@svsool
Copy link
Contributor Author

svsool commented Nov 22, 2018

My initial idea was to use convertExecutionResultToReason manually to map executionResult (JS Object) to executionResponse variant, same way as it's done here but with correct type, also that way it will be backward compatible and won't break code that already expect JS Object as mutation promise result (there is probably no such code, because type was wrong). But in general I think variant can be passed instead of plain object when mutation is called, if you mean Js.Promise.t(executionResponse('a)) instead of Js.Promise.t(executionResult).

@tmepple
Copy link

tmepple commented Nov 23, 2018

Even though it's a breaking change for the referenced code I definitely think we should be getting back the Variant when the mutation is called rather than having to always call the transformation function manually.

@Gregoirevda
Copy link
Contributor

Think so too, I'll add it this week end if I have time

@Gregoirevda Gregoirevda added this to PROGRESS in Fix them all Dec 7, 2018
@Gregoirevda Gregoirevda moved this from PROGRESS to TODO in Fix them all Dec 7, 2018
@Gregoirevda Gregoirevda moved this from TODO to PROGRESS in Fix them all Dec 7, 2018
@Gregoirevda
Copy link
Contributor

Fixed in #164 and merged

@Gregoirevda Gregoirevda moved this from PROGRESS to DONE in Fix them all Dec 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Fix them all
  
DONE
Development

Successfully merging this pull request may close these issues.

None yet

6 participants