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/SERV-12556-scenario-improvements #212

Conversation

PageTMcEneaney
Copy link

@PageTMcEneaney PageTMcEneaney commented Jun 1, 2023

Context

Summary

IP sprint contribution to add data/error to graphql response
At the moment, there is no option to add both data and error objects outside of the body. We'd like to add to support partial success graphql responses. See example. I want to make sure this is compatible with the parrot-friendly formatting of the scenarios so I think we should update the scenarios file to use or add a second example app

{
  "data": {
    "getInt": 12,
    "getString": null
  },
  "errors": [
    {
      "message": "Failed to get string!"
      // ...additional fields...
    }
  ]
}

Links

Issue: #210

Changes

Dev Notes

Steps to Test

Link local modules in examples/friendly-pirate-app/package.json to apply local changes

“parrot-friendly”: “file:../../packages/parrot-friendly”,
“parrot-graphql”: “file:../../packages/parrot-graphql”,

Screenshots

friendly-pirate-app

image image

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ PageTMcEneaney
❌ pmcenea


pmcenea seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@PageTMcEneaney PageTMcEneaney marked this pull request as ready for review June 13, 2023 18:46
@PageTMcEneaney PageTMcEneaney requested a review from a team as a code owner June 13, 2023 18:46
Comment on lines +65 to +75
// it('has full graphql success', () => {
// // graphql('/graphql', schema, {
// // Query: () => ({
// // Name: () => 'Jolly Roger',
// // Captain: () => 'Black Beard',
// // })
// // })
// graphql('/graphql', schema, () => ({
// ShipLog: () => { data: 'response' }
// }))
// });
Copy link
Author

Choose a reason for hiding this comment

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

I would appreciate some help with this mock, I want to prove that a graphql partial response is working as intended but was running into proxy issues.

Copy link
Member

Choose a reason for hiding this comment

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

Can you explain the issues you were having?

Comment on lines +35 to +44
data = value => {
this.structure.response.data = value;
return this;
};

errors = value => {
this.structure.response.errors = value;
return this;
};

Copy link
Author

Choose a reason for hiding this comment

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

These changes should support a partial response for non-graphql mocks

Comment on lines +40 to +42
if (resKeys.includes('data') || resKeys.includes('errors')) {
res.json(response);
}
Copy link
Author

Choose a reason for hiding this comment

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

These changes should return a partial success response (outside of {body: {}} object) for graphql calls

Comment on lines +18 to +29
// const graphql = require('parrot-graphql');

// const schema = `
// type Query {
// shiplog: [ShipLog]
// }

// type ShipLog {
// Name: String!
// Captain: String!
// }
// `;
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the comments if they aren't needed / adding context

@Matthew-Mallimo Matthew-Mallimo requested a review from a team June 14, 2023 13:27
@Matthew-Mallimo
Copy link
Member

Closing as this PR is stale

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