-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix: strengthen MockedResponse.newData type (#11584) #11592
Conversation
@Stephen2: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: ad70890 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Stephen2 👋
Thanks so much for this contribution! This is super helpful and I'm honestly surprised we didn't do this sooner. I just had a few comments for you in regards to structuring the tests that I'd like to see changed, but otherwise I'd love to get this in the next patch. Thanks a bunch!
@Stephen2 for the CircleCI Pipeline, I think you have to login there for those checks to run. Would you mind trying that? |
Just did this, created new account, signed in, but still wouldn't let me rerun this pipeline |
Took some liberties with your comments, but I think I captured the intention. Check it out again, ready for re-review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last round of comments! Thanks for bearing with me 🙂. This should be the last round of updates. I'd be happy to merge once addressed!
Dang I was hoping this would do it 😞. Just to double check, is the email address you're using here on GitHub the same as CircleCI? Let me also ask around and see if I can figure out why those checks won't run. |
Yeah, it definitely is. Maybe it's easier if you were to copy/paste these into a separate MR, or something, under your name? idk |
I'll try pushing a tiny edit to this branch and see if perhaps that will kick it off? So sorry for the inconvenience here! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setup
function still feels a tad overabstracted, but I can live with it. Thanks for making these updates! Don't worry about the extract API task failing. I'll get a commit pushed up to make this pass 🙂
@Stephen2 would you mind running |
Nah it's great, it'll grow on you. xD You're a good maintainer, I appreciate you. Hopefully we're ready to squash & merge now, except I think CircleCI needs to be kicked again :( |
Argh, that darn circle job. I'll get that kicked off again. As soon as thats green, I'm happy to merge this! We'll get this in the next patch release. And thanks so much for the kind comments 🙂. Means a lot! |
Checklist:
Closes #11584