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

Upgrade apollo and related packages #3304

Merged
merged 3 commits into from
Feb 11, 2022
Merged

Upgrade apollo and related packages #3304

merged 3 commits into from
Feb 11, 2022

Conversation

nickclyde
Copy link
Member

@nickclyde nickclyde commented Jan 28, 2022

Related Issue or Background Info

Changes Proposed

  • Upgrade @apollo/client to 3.4.17, the latest 3.4.x. The latest is 3.5.8, but upgrading to any 3.5.x broke our frontend unit tests, and I couldn't for the life of me figure out why. Changelog
  • Upgrade apollo-upload-client to 17.0.0. Changelog
  • Upgrade various @graphql-codegen packages, all minor version upgrades. Also regenerated the output. Changelog
  • Upgrade graphql to 16.3.0. Lots of breaking changes, but none seem to affect us. Changelog

Additional Information

  • decisions that were made
  • discussion of tradeoffs / future work
  • complaints about how bad the code is

Screenshots / Demos

Checklist for Author and Reviewer

Infrastructure

  • Consult the results of the terraform-plan job inside the "Terraform Checks" workflow run for this PR. Confirm that there are no unexpected changes!

Design

  • Any UI/UX changes have a designer as a reviewer, and changes have been approved
  • Any large-scale changes have been deployed to test, dev, or pentest and smoke-tested by both the engineering and design teams

Content

  • Any content changes (including new error messages) have been approved by content team

Support

  • Any changes that might generate new support requests have been flagged to the support team
  • Any changes to support infrastructure have been demo'd to support team

Testing

  • Includes a summary of what a code reviewer should verify

Changes are Backwards Compatible

  • Database changes are submitted as a separate PR
    • Any new tables that do not contain PII are accompanied by a GRANT SELECT to the no-PHI user
    • Any changes to tables that have custom no-PHI views are accompanied by changes to those views
      (including re-granting permission to the no-PHI user if need be)
    • Liquibase rollback has been tested locally using ./gradlew liquibaseRollbackSQL or liquibaseRollback
    • Each new changeset has a corresponding tag
  • GraphQL schema changes are backward compatible with older version of the front-end

Security

  • Changes with security implications have been approved by a security engineer (changes to authentication, encryption, handling of PII, etc.)
  • Any dependencies introduced have been vetted and discussed

Cloud

  • DevOps team has been notified if PR requires ops support
  • If there are changes that cannot be tested locally, this has been deployed to our Azure test, dev, or pentest environment for verification

@nickclyde nickclyde force-pushed the nick/upgrade-apollo branch from c713b64 to d03af15 Compare February 1, 2022 21:56
Base automatically changed from nick/upgrade-react-router to main February 2, 2022 02:07
@nickclyde nickclyde force-pushed the nick/upgrade-apollo branch from d03af15 to c2aacd9 Compare February 9, 2022 18:27
@sonarqubecloud
Copy link

sonarqubecloud bot commented Feb 9, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@fzhao99
Copy link
Contributor

fzhao99 commented Feb 10, 2022

Spent some time this morning digging into why 3.5.x is breaking our tests. Didn't get into a firm resolution but leaving findings here for posterity/in case it sparks anything for anyone.

It looks like the failing tests are generally failing because the mocks we're using on the relevant components aren't finding enough defined mocks as passed into the MockedProvider components we're using to simulate GraphQL calls. For example, from the MangedUsers.test.tsx file

image

Adding back in the specified mock in the error console (which in this case is just duplicating a mock that's already been defined) fixes the errors related to that issue.

Image 2-10-22 at 9 58 AM

There are some notes in the 3.5.0 changelog that describes changes to the way Apollo is handling the cache (which tbh I don't really understand) that might be related? Potentially defining those mocks once in 3.4.x and reading subsequent ones from the apollo cache (if this is how this works) might have changed in 3.5.0 given the changes. I might be way off base here though since I don't really understand much about the interactions at play here between RTL/Apollo, and React/GraphQL

Also worth noting that the above fixes/explanation only addresses some of the failures, so at best this is a half baked explanation 🙃

@nickclyde
Copy link
Member Author

Great digging Bob! I had seen the missing mock messages as well, but I didn't see the notes about changes to cache handling... I will take another peek there and see if anything stands out. Thanks for looking into it!!

@nickclyde nickclyde requested a review from jdorothy February 10, 2022 20:22
@zdeveloper zdeveloper temporarily deployed to Test February 10, 2022 20:56 Inactive
Copy link
Contributor

@emmastephenson emmastephenson left a comment

Choose a reason for hiding this comment

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

LGTM!

@nickclyde nickclyde merged commit 1553cf6 into main Feb 11, 2022
@nickclyde nickclyde deleted the nick/upgrade-apollo branch February 11, 2022 03:34
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.

Dependency updates: Apollo
5 participants