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

chore: 5397 Upgrade Next.js from v12 to v14 #6510

Merged
merged 5 commits into from
Jan 19, 2024
Merged

Conversation

tihuan
Copy link
Contributor

@tihuan tihuan commented Jan 12, 2024

Reason for Change

Changes

  1. Upgrade Next.js from 12 to 14
  2. Upgrade Blueprintjs from 4 to 5
  3. Update code to use the new next/image and next/link
  4. See inline PR comments for details
  5. Remove package next-dev-https, since Next.js now has their own dev server https mode
  6. Some Census Directory changes that might need @seve to review to make sure the code changes are good
  7. Some Blueprint Tooltip changes that might need @MillenniumFalconMechanic and @frano-m to take a look to make sure they're good

Known Issues

  1. Our Blueprint Popover usage will still cause React to throw Legacy Context API error, which I think is because Blueprint v5 Portal still supports the Legacy Context API and will only remove the support in v6
Screenshot 2024-01-18 at 7 51 32 AM

Testing steps

  1. The whole app should work as before

Checklist 🛎️

  • Add product, design, and eng as reviewers for rdev review
  • For UI changes, add screenshots/videos, so the reviewers know what you expect them to see
  • For UI changes, add e2e tests to prevent regressions

Notes for Reviewer

Copy link
Contributor

Deployment Summary

Copy link

codecov bot commented Jan 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0f05229) 92.12% compared to head (0ca8504) 92.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6510   +/-   ##
=======================================
  Coverage   92.12%   92.12%           
=======================================
  Files         180      180           
  Lines       14849    14849           
=======================================
  Hits        13679    13679           
  Misses       1170     1170           
Flag Coverage Δ
unittests 92.12% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tihuan tihuan force-pushed the thuang-5397-next-upgrade branch 4 times, most recently from d059e32 to 247b739 Compare January 17, 2024 19:01
@@ -100,3 +100,5 @@ src/views/CellGuide/common/fixtures/allTissues.json
src/views/CellGuide/common/fixtures/ontologyTree.json
src/views/CellGuide/common/fixtures/ontologyTreeStatePerCellType.json
src/views/CellGuide/common/fixtures/ontologyTreeStatePerTissue.json

certificates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nextjs built in https local FE server creates certs in this directory

Comment on lines +8 to +10
"@blueprintjs/core": "^5.8.2",
"@blueprintjs/icons": "^5.7.0",
"@blueprintjs/select": "^5.0.23",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blueprint upgraded from v4 to v5

@@ -101,7 +100,6 @@
"eslint-plugin-sonarjs": "^0.19.0",
"expect-playwright": "^0.8.0",
"gray-matter": "^4.0.3",
"next-dev-https": "0.2.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nexstjs now has built in https dev server mode

@@ -85,7 +85,7 @@ export const StyledCloseButtonIcon = styled(ButtonIcon, {
`;

export const NewsletterModal = styled(Modal)`
.bp4-dialog-header {
.bp5-dialog-header {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lots of bp4 -> bp5 changes. Here and throughout

@@ -1,5 +1,5 @@
import {
IMenuItemProps,
MenuItemProps,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Blueprint js API changes

Comment on lines +264 to +265
const sourceDataButton = page.getByTestId(SOURCE_DATA_BUTTON_ID);
const sourceDataList = page.getByTestId(SOURCE_DATA_LIST_ID);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

use the constants as single source of truth

Comment on lines -273 to -274
await page.keyboard.press("Escape");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update test steps here, since the new version of Blueprint doesn't work with Escape reliably

Comment on lines +33 to +38
"incremental": true,
"plugins": [
{
"name": "next"
}
]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actual change from Next.js upgrade

"**/*.ts",
"**/*.tsx",
"**/*.js",
".next/types/**/*.ts"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actual change from Next.js upgrade

@@ -5,24 +5,48 @@
"esModuleInterop": true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lots of prettier changes

@tihuan tihuan changed the title chore: Upgrade Next.js from v12 to v14 chore: 5397 Upgrade Next.js from v12 to v14 Jan 17, 2024
@tihuan tihuan marked this pull request as ready for review January 17, 2024 22:54
@tihuan tihuan requested review from MillenniumFalconMechanic and removed request for kaloster January 17, 2024 22:58
@@ -24,7 +25,7 @@ const AsyncCTA = loadable(
/*webpackChunkName: 'CreateCollectionModalCTA' */ import("./components/CTA")
);

const CreateCollectionButton = (props: Partial<IButtonProps>) => (
const CreateCollectionButton = (props: Partial<ButtonProps>) => (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice pick up thanks @tihuan! ⭐

popoverProps?: IPopoverProps;
buttonProps?: IButtonProps;
popoverProps?: PopoverProps;
buttonProps?: Partial<ButtonIconProps<"dotsHorizontal", "small">>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@tihuan thank you 🎆

@frano-m
Copy link
Collaborator

frano-m commented Jan 18, 2024

Hi @tihuan, I'm seeing a new warnings in my terminal. Are you seeing these too?

Warning: useLayoutEffect does nothing on the server, because its effect cannot be encoded into the server renderer's output format. This will lead to a mismatch between the initial, non-hydrated UI and the intended UI. To avoid this, useLayoutEffect should only be used in components that render exclusively on the client. See https://reactjs.org/link/uselayouteffect-ssr for common fixes.

And The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type". (home page only)?

@tihuan
Copy link
Contributor Author

tihuan commented Jan 18, 2024

Thanks so much for the thorough testing, @frano-m ! Sorry that I missed those warnings 🙏 I'll fix them today!

I can repro the NTag warning too 🙆‍♂️

Screenshot 2024-01-18 at 7 51 32 AM Screenshot 2024-01-18 at 7 51 45 AM

@tihuan
Copy link
Contributor Author

tihuan commented Jan 18, 2024

@frano-m I fixed the layout effect error (caused by using Blueprint AnchorButton) and replaced :first-childwith:first-of-type`, and the errors seemed to go away now!

The Blueprint5.Portal error I think it's because Blueprintv5 is still supporting React Legacy Context API and only marking it as deprecated. So I think the error naturally go away once Blueprint removes the support in v6!

@@ -24,58 +23,42 @@ export default function Nav({ className, pathname }: Props): JSX.Element {
<NavSection>
<NavSectionTitle>Application</NavSectionTitle>
<NavItemContainer>
<LinkWrapper>
<Link href={ROUTES.COLLECTIONS} passHref>
<AnchorButton
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@seve I removed AnchorButton usage since it violates React 18 usage. I think the nav link still works after I updated the style. But PTAL in case I missed anything 🙏 Thank you!

@tihuan
Copy link
Contributor Author

tihuan commented Jan 18, 2024

Thanks so much for the review, @seve ! Will wait for @frano-m to approve before merging 👍 !

Copy link
Collaborator

@frano-m frano-m left a comment

Choose a reason for hiding this comment

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

LGTM @tihuan! Nice work and thank you 🚀

@tihuan
Copy link
Contributor Author

tihuan commented Jan 19, 2024

Amazing! Thanks so much again for catching the errors, @frano-m 🤩 🙏 !

@tihuan tihuan merged commit 20c5864 into main Jan 19, 2024
40 checks passed
@tihuan tihuan deleted the thuang-5397-next-upgrade branch January 19, 2024 16:20
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.

None yet

3 participants