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

Upgrade nextjs #109

Merged
merged 5 commits into from
Jan 16, 2023
Merged

Upgrade nextjs #109

merged 5 commits into from
Jan 16, 2023

Conversation

prateek3255
Copy link
Contributor

Upgrade to Next.js 13 and apply relevant codemods using this guide:
https://nextjs.org/docs/upgrading

@prateek3255 prateek3255 requested a review from a team as a code owner January 15, 2023 09:17
@changeset-bot
Copy link

changeset-bot bot commented Jan 15, 2023

⚠️ No Changeset found

Latest commit: 0575038

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@gupta-ji6 gupta-ji6 left a comment

Choose a reason for hiding this comment

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

LGTM, asked few queries. I deployed it to staging as well, seems to be working fine - https://scriptified-technetium-co.vercel.app/

Side Note: We should also think upon updating other dependencies like next-pwa once.

@@ -92,6 +92,10 @@ function Curators(): JSX.Element {
width={150}
height={150}
alt={curator.name}
style={{
Copy link
Member

Choose a reason for hiding this comment

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

  1. these inline styles were added by codemod?
  2. the codemod didn't changed our imports?
The next/image import was renamed to next/legacy/image. The next/future/image import was renamed to next/image. A codemod is available to safely and automatically rename your imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ran to codemods, first one converted the imports to legacy imports and another experimental codemod that converts legacy imports to new imports and updates the syle according to the new API.
https://nextjs.org/docs/advanced-features/codemods

Comment on lines +7 to +8
<Link href="/" className="no-underline hover:underline">
← Back to home
Copy link
Member

Choose a reason for hiding this comment

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

also, I think we should redirect user to last visited page in the stack, not directly to Home.

If one goes to "All Issues" page, then open an issue, & then click this - it redirects back to home. not good UX.

let's take it up separately. I'll open an issue to remember.

@@ -1,10 +1,5 @@
/* eslint-disable @typescript-eslint/no-var-requires */
// eslint-disable-next-line @typescript-eslint/no-var-requires
// @ts-check
Copy link
Member

Choose a reason for hiding this comment

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

why did we removed this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was getting a bunch of type errors maybe because of the withPWA conflicting with the original types. We can fix this separately though

@@ -21,13 +22,13 @@
"axios": "0.21.2",
"glob": "7.1.6",
"lodash": "4.17.21",
"next": "12.2.0",
"next": "13.1.2",
Copy link
Member

Choose a reason for hiding this comment

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

🚀

@gupta-ji6 gupta-ji6 merged commit b5b5185 into main Jan 16, 2023
@gupta-ji6 gupta-ji6 linked an issue Jan 16, 2023 that may be closed by this pull request
@gupta-ji6 gupta-ji6 added the enhancement New feature or request label Jan 16, 2023
@prateek3255 prateek3255 deleted the upgrade-nextjs branch January 17, 2023 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Upgrade to Next.js 13
2 participants