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

Upgade to Next.js 15 and React 19 #5475

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Upgade to Next.js 15 and React 19 #5475

wants to merge 5 commits into from

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Jan 9, 2025

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)

  • Localization strings (if needed) have been added.
  • Commits in this PR are minimal and have descriptive commit messages.
  • I've added or updated the relevant sections in readme and/or code comments
  • I've added a unit test to test for potential regressions of this bug. - N/A, existing tests apply.
  • If this PR implements a feature flag or experimentation, the Ship Behind Feature Flag status in Jira has been set
  • Product Owner accepted the User Story (demo of functionality completed) or waived the privilege.
  • All acceptance criteria are met.
  • Jira ticket has been updated (if needed) to match changes made during the development process.
  • Jira ticket has been updated (if needed) with suggestions for QA when this PR is deployed to stage.

Vinnl added 4 commits January 8, 2025 14:15
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.)
@Vinnl Vinnl added the Review: S Code review time: 30 mins to 1 hour label Jan 9, 2025
@Vinnl Vinnl requested review from rhelmer, codemist and flozia January 9, 2025 16:50
@Vinnl Vinnl self-assigned this Jan 9, 2025
@@ -386,7 +387,7 @@ export const View = (props: Props) => {
typeof props.totalNumberOfPerformedScans === "undefined" ||
props.totalNumberOfPerformedScans <
CONST_ONEREP_MAX_SCANS_THRESHOLD ? (
<a
<Link
Copy link
Collaborator Author

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, {});
Copy link
Collaborator Author

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

Copy link
Collaborator Author

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.

Copy link

github-actions bot commented Jan 9, 2025

Base automatically changed from sass-migrator to main January 9, 2025 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: S Code review time: 30 mins to 1 hour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant