bloom-frontend icon indicating copy to clipboard operation
bloom-frontend copied to clipboard

NextJS: Upgrade From Pages to App

Open kyleecodes opened this issue 1 year ago • 10 comments

Overview

NextJS Apps is the upgraded version of NextJS Pages. We should leverage the additional features and benefits by upgrading Bloom's frontend to NextJS Apps.

Benefits of NextJS Apps include but not limited to: enhanced scalability from improved modular approaches, advanced routing capabilities, improved performance in server-side rendering and static site generation, built-in optimizations for high traffic.

Action Items

Note: Please document the changes to be made with the upgrades. If there are many or highly complex changes, we will open new issues for them.

Resources/Instructions

kyleecodes avatar Nov 28 '23 21:11 kyleecodes

@kyleecodes please assign it to me.

This one I would recommend to split it in several sub tasks/issues as follow:

  1. Move app/ folder content to store/ folder (the app router uses app/ folder as convention to have the pages and layouts, I think moving any other logic outside is recommended as drives to a cleaner and more focused structure #1019
  2. Copy _app.tsx and _document.tsx within a root layout #1040
  3. Iteratively migrate pages to app router, one task/issue per migration. A possible unordered sequence would be:
  • meet the team #1040
  • account
  • admin
  • api
  • auth
  • courses
  • partner-admin
  • partnership
  • subscription
  • therapy
  • welcome #1075
  • error pages
  • rest of root pages within pages/ folder

For each migration, ideally I would include:

  • The migration from pages to app router following the recommended approach
  • An E2E test if not available to cover the happy path of the feature/page
  • A proposal of a nested layout and/or folder structure, migration to server component, next/head, routing hooks, data fetchting methods and styling among others to discuss and tackle in a follow up issue
  1. Remove _app.tsx and _document.tsx

boodland avatar Jun 17 '24 11:06 boodland

@eleanorreem or @annarhughes can you have a look to my previous comment? I would like to work on this issue. Thanks!

boodland avatar Jul 01 '24 08:07 boodland

Hey @boodland thank you for your patience. These suggestions are helpful, thank you.

Since this is a more ambitious contribution, we recommend starting with a beginner issue first to familiarize yourself. Would you be open to being assigned this issue to start: https://github.com/chaynHQ/bloom-frontend/issues/918? Comment to be assigned there and we'll get you started.

kyleecodes avatar Jul 02 '24 22:07 kyleecodes

Hi @kyleecodes, sure thing, I will be delighted to work on #918, but please keep me into account for this one if possible as I would love to do it.

boodland avatar Jul 03 '24 10:07 boodland

Thank you @boodland you have been assigned this issue! Please follow the directions in our Contributing Guide. We look forward to reviewing your pull request shortly ✨


Support Chayn's mission? ⭐ Please star this repo to help us find more contributors like you! Learn more about Chayn here and explore our projects. 🌸

github-actions[bot] avatar Jul 10 '24 16:07 github-actions[bot]

@eleanorreem We want to check what your input is on this?

kyleecodes avatar Jul 10 '24 16:07 kyleecodes

@boodland sorry for the delay and thank you for your patience. I was speaking with one of my team mates @annarhughes who has previously done this migration. They suggested these points:

  1. Follow this guide on upgrading to new features, such as the improved Image and Link Components: NextJS Apps: New Feature Guide. (as in the original issue description)
  2. Move app/ folder content to store/ folder
  3. Copy _app.tsx and _document.tsx within a root layout
  4. Iteratively migrate pages to app router, one task/issue per migration:
  5. include the new next/head, and routing hooks - i.e. all new features should be used in each page migration
  6. exclude data fetching and styling for now (suggestions/proposals for implementations welcome). Existing redux (RTK & RTK query) and MUI packages are deeply rooted and used throughout the app, easy improvements towards server components would be great, however major changes like removing RTK/MUI might not be worth the performance gains for us
  7. Remove _app.tsx and _document.tsx

Otherwise, all good to go. @boodland do you need me to create an issue for each step that you take? or are you happy to write up an issue for each page you migrate/ step that you take?

Thanks again for your input.

eleanorreem avatar Jul 14 '24 11:07 eleanorreem

@boodland sorry for the delay and thank you for your patience. I was speaking with one of my team mates @annarhughes who has previously done this migration. They suggested these points:

  1. Follow this guide on upgrading to new features, such as the improved Image and Link Components: NextJS Apps: New Feature Guide. (as in the original issue description)
  2. Move app/ folder content to store/ folder
  3. Copy _app.tsx and _document.tsx within a root layout
  4. Iteratively migrate pages to app router, one task/issue per migration:
  5. include the new next/head, and routing hooks - i.e. all new features should be used in each page migration
  6. exclude data fetching and styling for now (suggestions/proposals for implementations welcome). Existing redux (RTK & RTK query) and MUI packages are deeply rooted and used throughout the app, easy improvements towards server components would be great, however major changes like removing RTK/MUI might not be worth the performance gains for us
  7. Remove _app.tsx and _document.tsx

Otherwise, all good to go. @boodland do you need me to create an issue for each step that you take? or are you happy to write up an issue for each page you migrate/ step that you take?

Thanks again for your input.

@eleanorreem and @annarhughes thanks for your feedback!

I have been looking at point 1 and seems to me that the new Image component is already used as there are no references to the /next/legacy/image import, so all the imports are using the new one. For the link one I have tracked where it is used and is wrapped within the custom common/Link component which doesn't seem to pass an anchor element as child which is the change that can be avoid using the new Link component from Next.js 13. although I cannot say it 100% as it is referenced from many places but the ones I have checked are just using the Link component as value for the component property in other components, so I am going to assume point 1 is fine but keep an eye when doing the migration of the individual pages in case something can be changed related to them.

With respect to point 5 migrating to new next/head and routing hooks I will include them in each page migration if possible, not sure if can be easily added as we will be creating a client page component to maintain consistency with pages approach and importing it within the page server component of the app route but I would say it should be fine to change both during the page migration.

I will add comments about point 6, as proposed, in the follow up issue of how to approach the migration to server component if possible.

@eleanorreem No need to create new issues for each sub item/issue, I have already done myself for the relocation of the store logic #1019 and I can do it for the rest that I will be working on but if you want to create them so they are all visible, please go ahead

Let me know if there is anything I misunderstood.

boodland avatar Jul 15 '24 12:07 boodland

@boodland your comments makes sense for me. I haven't done the migration before and @annarhughes is off for a bit so go ahead and we can take it a ticket at a time!

eleanorreem avatar Jul 17 '24 15:07 eleanorreem

As per Chayn policy, after 30 days of inactivity, we will be unassigning this issue. Please comment to stay assigned.

github-actions[bot] avatar Sep 16 '24 00:09 github-actions[bot]