-
Notifications
You must be signed in to change notification settings - Fork 221
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
Upgade to Next.js 15 and React 19 #5475
base: main
Are you sure you want to change the base?
Conversation
This fixes Sass deprecation warnings by running npx sass-migrator division **/*.scss and npx sass-migrator module **/*.scss The only manual change after that was replacing `math.div` with `list.slash` in the `$text-title-*` and `$text-body-*` variables in tokens.scss, as the `/` there wasn't part of a division.
This upgrades Next.js by running `npx @next/codemod@canary upgrade latest`, which also runs `npx codemod@latest react/19/migration-recipe`. Additional manual changes I had to do: 1. Disable Turbopack in dev mode. It wasn't clear to me how to use raw-loader to load .ftl files, so I postponed that for later. 2. Turned `getExperimentationId` into an async function. I did this because it calls `headers()`, which is async now. All its invocations where in async functions anyway. 3. Server components should now pass the accept language to get l10n In most environments, getL10n() and getL10nBundles() can be synchronous functions, and just read the language preferences directly. However, on the server-side, reading the headers is now asynchronous. Thus, I've modified the l10n generation functions to require the accept language argument if no sync function can be provided that returns it. Not great, but better than forcing the same functions in e.g. client components to be async. An alternative would have been to simply make the getL10n() and getL10nBundles() functions asynchronous for the server, but I imagine that would be even more confusing than just making an optional argument required. 4. Dynamically import react-dom/server(.edge) In `/src/emails/renderEmail`, we _want_ to render HTML to a string, rather than generating a server component that the server serves, but Next.js throws an error if we do that directly. Hence, I had to replace that with a dynamic import as a workaround, turning `renderEmail` into an async function. Additionally, I got errors in the tests when not importing it from .edge. 5. Move `serverExternalPackages` out of `experimental`
After the Next.js update, these two newly showed up as linting errors.
By using `serverExternalPackages`, we should be ready for when we switch to Turbopack. (But we can't switch to Turbopack yet because, as far as I know, it doesn't support require.context yet, which we use for discovering FTL files.)
@@ -386,7 +387,7 @@ export const View = (props: Props) => { | |||
typeof props.totalNumberOfPerformedScans === "undefined" || | |||
props.totalNumberOfPerformedScans < | |||
CONST_ONEREP_MAX_SCANS_THRESHOLD ? ( | |||
<a | |||
<Link |
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.
@flozia This came out of a new linting error, presumably a bugfix in a parser or something. You added the initial <a>
- do you remember if that was a conscious decision, or an oversight?
@@ -85,17 +90,17 @@ const EmailAddressAddForm = () => { | |||
const l10n = useL10n(); | |||
const recordTelemetry = useTelemetry(); | |||
const formRef = useRef<HTMLFormElement>(null); | |||
const [formState, formAction] = useFormState(onAddEmail, {}); | |||
const [onAddEmailState, onAddEmailAction] = useActionState(onAddEmail, {}); |
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.
useFormState
was renamed to useActionState
: facebook/react#28491
src/app/functions/l10n/index.ts
Outdated
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.
Some context for the changes here: the l10n initialisation code differs per environment (see the other files in this directory). Each environments calls the functions in this file to create the same APIs: getL10nBundles
and getL10n
.
One of the functions that is different for every environment is how to read the Accept-Lang header. However, the function to do this is synchronous, which is now problematic in server components, which need to await headers()
to get it since Next.js v15.
So instead of calling that API automatically, server components now need to load the Accept-Lang header beforehand, and then pass it explicitly into getL10n
.
The changes in this file enable that: when the environment-specific code does not provide a function to auto-detect the Accept-Language (i.e. getAcceptLangHeader
), the resulting getL10n
function will have it as a required argument.
Preview URL 🚀 : https://blurts-server-pr-5475-mgjlpikfea-uk.a.run.app |
References:
Jira: MNTOR-3861
Description
See https://nextjs.org/blog/next-15 and https://react.dev/blog/2024/12/05/react-19.
The individual commit messages show what I had to do exactly, most importantly 40536c2. Most of it was done through codemods, but I had to do some manual work as well. The most painful bit was all Next.js request-related methods (e.g.
headers()
) now being asynchronous.Unfortunately I couldn't enable Turbopack for dev mode yet, because it doesn't support
require.context
as far as I could see, which we use to get a listing of available locales with.ftl
files.How to test
There should primarily be no regressions. We'll probably want to coordinate with QA on when to merge this.
Checklist (Definition of Done)