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

fix: strengthen MockedResponse.newData type (#11584) #11592

Merged
merged 6 commits into from Feb 15, 2024
Merged

fix: strengthen MockedResponse.newData type (#11584) #11592

merged 6 commits into from Feb 15, 2024

Conversation

Stephen2
Copy link
Contributor

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

Closes #11584

@Stephen2 Stephen2 requested a review from a team as a code owner February 13, 2024 17:18
@apollo-cla
Copy link

@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/

Copy link

netlify bot commented Feb 13, 2024

👷 Deploy request for apollo-client-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit ad70890

Copy link

changeset-bot bot commented Feb 13, 2024

🦋 Changeset detected

Latest commit: ad70890

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

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

Copy link
Member

@jerelmiller jerelmiller left a 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!

src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
.changeset/dirty-insects-return.md Outdated Show resolved Hide resolved
@jerelmiller
Copy link
Member

@Stephen2 for the CircleCI Pipeline, I think you have to login there for those checks to run. Would you mind trying that?

@Stephen2
Copy link
Contributor Author

@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

@Stephen2
Copy link
Contributor Author

Took some liberties with your comments, but I think I captured the intention. Check it out again, ready for re-review

Copy link
Member

@jerelmiller jerelmiller left a 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!

src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
src/testing/core/mocking/__tests__/mockLink.ts Outdated Show resolved Hide resolved
@jerelmiller
Copy link
Member

Just did this, created new account, signed in, but still wouldn't let me rerun this pipeline

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.

@Stephen2
Copy link
Contributor Author

Stephen2 commented Feb 14, 2024

Just to double check, is the email address you're using here on GitHub the same as CircleCI?

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

@jerelmiller
Copy link
Member

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!

Copy link
Member

@jerelmiller jerelmiller left a 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 🙂

@jerelmiller
Copy link
Member

@Stephen2 would you mind running npm run format and pushing that change? I'd be happy to do it if need be, but I'd prefer you get credit for those lines of code written 🙂

@Stephen2
Copy link
Contributor Author

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 🙂

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 :(

@jerelmiller
Copy link
Member

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!

@jerelmiller jerelmiller merged commit 1133469 into apollographql:main Feb 15, 2024
29 checks passed
@github-actions github-actions bot mentioned this pull request Feb 15, 2024
@Stephen2 Stephen2 deleted the issue-11584 branch February 15, 2024 18:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better types MockedResponse.newData
3 participants