-
Notifications
You must be signed in to change notification settings - Fork 16
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
[Issue #3434] React USWDS, React, Next.JS upgrade #3515
Conversation
@@ -2,4 +2,4 @@ | |||
/// <reference types="next/image-types/global" /> | |||
|
|||
// NOTE: This file should not be edited | |||
// see https://nextjs.org/docs/app/building-your-application/configuring/typescript for more information. | |||
// see https://nextjs.org/docs/app/api-reference/config/typescript for more information. |
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.
This was autogen
@@ -22,7 +21,7 @@ export type ValidationErrors = { | |||
export default function SubscriptionForm() { | |||
const t = useTranslations("Subscribe"); | |||
|
|||
const [state, formAction] = useFormState(subscribeEmail, { | |||
const [state, formAction] = useActionState(subscribeEmail, { |
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.
@@ -61,7 +61,8 @@ export const createSession = async (token: string) => { | |||
} | |||
const expiresAt = newExpirationDate(); | |||
const session = await encrypt(token, expiresAt, clientJwtKey); | |||
cookies().set("session", session, { | |||
const cookie = await cookies(); |
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.
Dynamic APIs are:
- The params and searchParams props that get provided to pages, layouts, metadata APIs, and route handlers.
- cookies(), draftMode(), and headers() from next/headers
In Next 15, these APIs have been made asynchronous. You can read more about this in the Next.js 15 Upgrade Guide.
27d030f
to
ae3527a
Compare
I've got tests working for upgrading to React 19 and Next.JS 15. Currently working on: ✅ 2. Errors in the console compiling SCSS⚠ ./src/styles/styles.scss.webpack[javascript/auto]!=!./node_modules/next/dist/build/webpack/loaders/css-loader/src/index.js??ruleSet[1].rules[13].oneOf[11].use[2]!./node_modules/next/dist/build/webpack/loaders/postcss-loader/src/index.js??ruleSet[1].rules[13].oneOf[11].use[3]!./node_modules/next/dist/build/webpack/loaders/resolve-url-loader/index.js??ruleSet[1].rules[13].oneOf[11].use[4]!./node_modules/next/dist/compiled/sass-loader/cjs.js??ruleSet[1].rules[13].oneOf[11].use[5]!./src/styles/styles.scss
Deprecation Warning on line 13, column 6 of file:///Users/partisan/workshop/grants/simpler-grants-gov2/frontend/node_modules/@uswds/uswds/packages/uswds-core/src/styles/functions/units/rem-to-user-em.scss:13:6:
Global built-in functions are deprecated and will be removed in Dart Sass 3.0.0.
Use math.unit instead.
More info and automated migrator: https://sass-lang.com/d/import
13 | @if unit($grid-in-rem) != "rem" {
node_modules/@uswds/uswds/packages/uswds-core/src/styles/functions/units/rem-to-user-em.scss 14:7 rem-to-user-em()
node_modules/@uswds/uswds/packages/uswds-core/src/styles/mixins/helpers/at-media.scss 17:12 at-media()
node_modules/@uswds/uswds/packages/usa-display/src/styles/_usa-display.scss 7:3 @forward
node_modules/@uswds/uswds/packages/usa-display/src/styles/_index.scss 4:1 @forward
node_modules/@uswds/uswds/packages/usa-display/_index.scss 5:1 @forward
node_modules/@uswds/uswds/packages/uswds-typography/_index.scss 5:1 @forward
node_modules/@uswds/uswds/packages/uswds/_index.scss 13:1 @forward
src/styles/styles.scss 2:1 root stylesheet ✅ Better documenting PR / changes |
b1a883b
to
cfb20a7
Compare
5b726ca
to
4dadf33
Compare
"@storybook/addon-designs", | ||
"@chromatic-com/storybook", | ||
], | ||
addons: ["@storybook/addon-essentials", "@chromatic-com/storybook"], |
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.
Temp remove @storybook/addon-designs
until it is React 19 compatible
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.
sounds good - are we actively using this in any case, or can we just remove it for good? Since there's not even an issue to add React 19 support I wouldn't be too confident that it'll come any time soon
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.
In practice it is "removed for good" as it is removed from npm and there is no config left.
experimental: { | ||
serverComponentsExternalPackages: ["newrelic"], | ||
}, | ||
serverExternalPackages: ["newrelic"], |
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.
params: { locale }, | ||
}: LocalizedPageProps) { | ||
export async function generateMetadata({ params }: LocalizedPageProps) { | ||
const { locale } = await params; |
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.
Lots of changes due to Async Request APIs (Breaking change)
@@ -1,5 +1,3 @@ | |||
import React from "react"; |
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.
No longer needed, removed when running codemon upgrade script.
frontend/src/hoc/withFeatureFlag.tsx
Outdated
props: P & WithFeatureFlagProps, | ||
) => { | ||
const searchParams = props.searchParams || {}; | ||
const searchParams = (await props.searchParams) || {}; | ||
const cookiesRead = await cookies(); |
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.
@doug-s-nava used cookiesRead
and *Read
a couple of times. Not wedded to that.
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.
not a blocker but I think something like "resolvedCookies" would be more easily understandble
@@ -10,7 +10,7 @@ in the form $setting: value, | |||
@use "uswds-core" with ( | |||
// Disable scary but mostly irrelevant warnings: | |||
$theme-show-notifications: false, | |||
|
|||
$theme-show-compile-warnings: false, |
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.
Necessary to suppress a number of Sass warnings about upcoming versions. We are tied to uswds which is where a lot of them originated.
@@ -25,6 +25,13 @@ const mockSetFeatureFlag = jest.fn( | |||
}, | |||
); | |||
|
|||
jest.mock("react", () => ({ | |||
...jest.requireActual<typeof import("react")>("react"), | |||
use: jest.fn(() => ({ |
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.
Necessary for pages that "use" use()
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.
a larger discussion for another time probably, but with stuff like this would it make more sense to mock it globally?
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.
I created a ticket: #3657
@@ -0,0 +1,30 @@ | |||
import { render, screen } from "@testing-library/react"; |
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.
Ended up adding this in order to test an async page.
@@ -96,6 +96,7 @@ describe("getSession", () => { | |||
describe("createSession", () => { | |||
afterEach(() => jest.clearAllMocks()); | |||
// to get this to work we'd need to manage resetting all modules before the test, which is a bit of a pain | |||
// eslint-disable-next-line jest/no-disabled-tests | |||
it.skip("initializes session secrets if necessary", async () => { |
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.
These show up locally if you run the linter.
const OriginalComponent = jest.fn(); | ||
const searchParams = { any: "param" }; | ||
const WrappedComponent = withFeatureFlag( | ||
OriginalComponent, | ||
"searchOff", | ||
identity, | ||
); | ||
render(<WrappedComponent searchParams={searchParams} />); |
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.
This is a little gnarly. The FF wrapper is now async since it uses cookies and takes params.
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.
Yeah, this component in general is pretty gnarly. I'd welcome just ignoring typescript on this file if that'd make things easier. Maybe just make it a js file, or do we have a rule against that?
f8fb701
to
ef60f39
Compare
ef60f39
to
a692ad5
Compare
84b6bac
to
2574e3e
Compare
Summary
Fixes #3434
Time to review: 45 mins
Changes proposed
Upgrade to React 19, Next 15, and the following
packages
acouch/issue-3434-uswds-react-upgrade
main
Temporarily removes
@storybookjs/addon-designs
Since it is not yet react 19 compatible.
USWDS Upgrade
There are some changes upgrading uswds 3.8.0 to 3.11.0.
This results from a breaking change from this PR: uswds/uswds#5636
from:
to:
I've left this as is to stay aligned with USWDS.
There was a change in padding in the header from this breaking PR: uswds/uswds#6037 that fixes an issue with the ordering of the search bar which we don't use.
After upgrading USWDS, the header has an extra
.25rem
padding:I overwrote that change for simpler in this PR: https://github.com/HHS/simpler-grants-gov/pull/3515/files#diff-992305390a5bf9f2ade09df84638c6473249440c5612ae3a328b1b174aa6b7edR439
But can remove that if you'd rather stay up-to-date with USWDS.
Context for reviewers