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

Update react-scripts to 5.0.0 #3209

Merged
merged 24 commits into from
Jan 14, 2022
Merged

Update react-scripts to 5.0.0 #3209

merged 24 commits into from
Jan 14, 2022

Conversation

nickclyde
Copy link
Member

@nickclyde nickclyde commented Jan 11, 2022

Related Issue or Background Info

  • Part of Dependency updates: React #2908
  • We are actually already on the latest stable version of react, 17.0.2. However, React 18 is in rc and release is imminent. I will be keeping an eye out to see if it releases in the next few days.
  • In the meantime, this PR updates react-scripts, along with a slew of other packages that need updating along with it, including uswds
  • Resolves Dependency updates: USWDS #2909

Changes Proposed

  • Update react-scripts to 5.0.0
  • Removed craco. We only use it to allow sass mixins in component sass files, which we only did in one file. See Get Sass resources to work when imported into the component #1676 and Update to dart-sass and use mixins at the component level #1681. I decided to remove since it reduces our build complexity, and we don't really have any sass/uswds experts on the team anymore, so it seems unlikely that we will use mixins anyways.
  • Update testing library packages since react-scripts upgraded jest
  • Fix three tests that broke after the upgrades, luckily nothing too crazy needed to change
  • Since react-scripts also updated eslint to 8.x, needed to update some eslint plugins and change some eslint configuration, including adding a typescript parser for it, see more here
  • Because of the update to webpack 5, needed to update uswds to 2.13.0 as well. Along with this I had to change the way we import some uswds icons
  • Update sass to 1.47.0. Along with the uswds upgrade this seems to have finally cleared out all the useless deprecation warnings and changelog notes we get when running the frontend!! 🎉
  • Updated typescript to 4.5.4, and fixed a whole bunch of ts errors in our tests
  • Update various storybook packages so that storybook can use webpack 5
  • Deleted some components that we never ended up using (for okta login and password reset)

Additional Information

  • The updates seemed to change our CSS output in a few places, so I'm working through making sure there are no UI changes (thanks Chromatic!)

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

@sonarcloud
Copy link

sonarcloud bot commented Jan 13, 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

@nickclyde nickclyde changed the title [WIP] Update react-scripts to 5.0.0 Update react-scripts to 5.0.0 Jan 13, 2022
@nickclyde
Copy link
Member Author

This should be ready for review now! There were 8 UI changes in Chromatic, but I honestly couldn't tell the difference by visual inspection, so I just went ahead and accepted them. I'm gonna go ahead and deploy to dev and do some smoke testing to make sure nothing looks off.

@nickclyde nickclyde temporarily deployed to Dev January 13, 2022 22:09 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!

@@ -48,7 +48,7 @@ export const useFacilityValidation = (facility: Facility) => {
orderingProviderIsRequired: requiresOrderProvider(facility.state),
},
});
} catch (e) {
} catch (e: any) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for getting rid of these red squiggles in VSCode 🙂

@@ -4,12 +4,12 @@ describe("isValidCLIANumber::string -> boolean", () => {
test("valid number returns `true`", () => {
const cliaNumber = "12D4567890";

expect(isValidCLIANumber(cliaNumber)).toBe(true);
expect(isValidCLIANumber(cliaNumber, "VA")).toBe(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor

@nathancrtr nathancrtr left a comment

Choose a reason for hiding this comment

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

So many great improvements Nick, thanks for this! 🚀

There are some CSS changes that I can't visualize but this LGTM assuming happy results from your smoke testing & Chromatic review.

Copy link
Collaborator

@rin-skylight rin-skylight left a comment

Choose a reason for hiding this comment

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

LGTM! I also checked the terraform plan output, and things look stable from an infrastructure perspective. Thank you for this huge amount of work!

@nickclyde nickclyde merged commit 0e893a8 into main Jan 14, 2022
@nickclyde nickclyde deleted the nick/update-react-scripts branch January 14, 2022 19:39
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: USWDS
4 participants