-
Notifications
You must be signed in to change notification settings - Fork 1
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
[MPDX-8445] NextJS v13 upgrade #1204
Conversation
* @mui/material, @mui/icons-material, and lodash are optimized by default https://nextjs.org/docs/13/app/api-reference/next-config-js/optimizePackageImports * Fix webpack rule matcher
Preview branch generated at https://8445-nextjs-13-upgrade.d3dytjb8adxkk5.amplifyapp.com |
Bundle sizes [mpdx-react]Compared against e5e950c
|
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.
The code looks great! Thank you for doing this. I have reviewed all the code apart from the changes about 45 mins ago. I have also reviewed it on the Amplify preview URL.
It looks good. I had some comments, and I still need to review your new changes.
src/components/Contacts/ContactDetails/ContactDetailsHeader/ContactDetailsHeader.tsx
Outdated
Show resolved
Hide resolved
src/components/Dashboard/ThisWeek/LateCommitments/LateCommitments.tsx
Outdated
Show resolved
Hide resolved
src/components/Layouts/Primary/TopBar/Items/SearchMenu/SearchDialog.tsx
Outdated
Show resolved
Hide resolved
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 looking good. I have a few comments, but no big changes. I will approve so you are not blocked.
Do you also want to change pages/accountLists/[accountListId]/contacts/[[...contactId]].page.tsx
title to match how you have done the titles on other pages? I'm not too fussed about it
<title>
{appName} |{' '}
{viewMode === TableViewModeEnum.Flows
? t('Contact Flows')
: viewMode === TableViewModeEnum.Map
? t('Contacts Map')
: t('Contacts')}
</title>
```
NextJS automatically loads .env files into process.env
Fixes "Warning: A title element received an array with more than 1 element as children."
4c97ea1
to
6087e61
Compare
Description
Upgrade to NextJS v13. The main change was changing all of our
next/link
components (migration guide). Links now create ana
element instead of needing to have a single child that is ana
element. In the future we no longer have to usepassHref
orlegacyBehavior
(and we should not use either of those props in the future).Other changes
next.config.js
file and fixed warnings about environment variables being undefined.next-image-loader
webpack rule.experimental.modularizeImports
configuration (https://nextjs.org/docs/13/app/api-reference/next-config-js/optimizePackageImports).dotenv
because NextJS already supports automatically loading.env
files (https://nextjs.org/docs/13/app/building-your-application/configuring/environment-variables).<title>
elements to use string interpolation instead of JSX expressions. This fixes theWarning: A title element received an array with more than 1 element as children.
warnings and fixes tab titles briefly containing<!-- -->
.Amplify config changes
NODE_ENV
environment variable. Setting it todevelopment
caused build errors, and the Next.js docs advise against this. It is automatically set toproduction
by Next.js when runningyarn build
, so we don't need to manually set it.package.json
.MPDX-8445
Checklist: