chaynHQ / bloom-frontend

Code for the for the frontend of the Bloom service.
https://bloom.chayn.co/
MIT License
28 stars 46 forks source link

Migrate move app and document files to root layout, migrate meet the team page and create test of the page #1064

Open boodland opened 2 months ago

boodland commented 2 months ago

Migrated _document.tsx and _app.tsx to app router with a root layout Migrated Meet The Team page to app router in order to test the root layout and added test

Issue link / number:

1040 sub issue of #702

What changes did you make? Migrated _document.tsx and _app.tsx configuration which implies to:

Why did you make the changes? In order to be able to migrate the product from pages router to app router and benefits of all the new features available in the app router

Did you run tests? Yes

vercel[bot] commented 2 months ago

@boodland is attempting to deploy a commit to the Chayn Team on Vercel.

A member of the Team first needs to authorize it.

eleanorreem commented 2 months ago

Hi @boodland sorry for not getting back to you on this! So much work ! We've had a Reading Week so normal work was parked. I am also off the coming week as well. Hopefully we can some feedback for you for when you are back from holiday.

boodland commented 2 months ago

@eleanorreem no worries, I will be busy as well this week with some interviews, once I have more time I will update the branch and carry on with the rest of the migration pages branching from this one. I will rebase once it is in the main branch.

Enjoy your week off

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ā†—ļøŽ

Name Status Preview Comments Updated (UTC)
bloom-frontend āŒ Failed (Inspect) Aug 13, 2024 4:25pm
eleanorreem commented 2 months ago

@boodland thanks so much for this. Thanks for putting so much time into this and your comments have been super helpful in navigating the PR. There are a couple things it would be good to address initially:

@annarhughes can do an indepth review.

annarhughes commented 2 months ago

Hey @boodland thanks for such a huge effort on this migration so far šŸŒŸ I saw the comments re challenges on dynamic routes and the integrations (MUI etc) and agree there are some tricky bits to this! For that reason and for reviewing purposes, I'd suggest we break down this work into smaller PRs, so we can review and support on each piece. We can merge each minor PR into a parent app-router-migration PR for now, until _app and _document are safely replaced.

We could break down PRs in this order, roughly following the order of steps from the nextjs guide:

Apologies for not being more explicit about these steps, hoping they can help us move faster going forward šŸ™

boodland commented 2 months ago

Hey @boodland thanks for such a huge effort on this migration so far šŸŒŸ I saw the comments re challenges on dynamic routes and the integrations (MUI etc) and agree there are some tricky bits to this! For that reason and for reviewing purposes, I'd suggest we break down this work into smaller PRs, so we can review and support on each piece. We can merge each minor PR into a parent app-router-migration PR for now, until _app and _document are safely replaced.

We could break down PRs in this order, roughly following the order of steps from the nextjs guide:

  • Step 1: Creating the app directory and Step 2: Creating a Root Layout but creating a basic root layout without integrations (MUI, redux) etc
  • Create a new PR for each integration in app router/root layout (MUI, redux, Intl/localisation, storyblok, hotjar, crisp). For each integration, check the docs for the new implementation using app router - many integrations have alternative functions and implementations provided, so we shouldn't just copy/paste the existing solution from _app _document. Provide details of the changes/challenges in each PR and supporting docs. Note: Redux does not work with server components - we will likely remove redux in the future and for now will use client components as described in the guide
  • Create a PR to add the AuthGuards components/functionality to the root layout and ensure they're working
  • Create a PR to add any remaining logic from the old _app and _document files to the new layout files, before deleting them
  • Step 3: Migrating next/head - can be completed for the root layout here, and for each page going forward
  • Step 5: Migrating Routing Hooks - can be completed for the root layout here, and for each page going forward
  • Step 4: Migrating Pages - one PR per page. Once the first page is complete, merge app-router-migration branch before creating next page PRs

Apologies for not being more explicit about these steps, hoping they can help us move faster going forward šŸ™

Hi @annarhughes, I completely agree, I wasn't aware of the complexity of the migration as I wasn't familiar with the codebase.

I think your suggestion of using a parent app-router-migration PR to have the changes is mandatory as I don't see an easy (probably very very difficult) way of having both router versions (Pages and App) working together with i18n due to the current complexity, so having this parent PR is going to allow us to progressively migrate the logic and the pages without having to support both approaches, the idea is to have working App router with i18n routing which is very easy to achieve while Pages router wouldn't work, once all pages are migrated the PR should be able to be merged within dev.

Let me know if I should consider anything else.

annarhughes commented 2 months ago

Thanks for the quick response @boodland ! My thinking was to use a parent branch only up to the point of the app/layout being created, and a first successful page. I.e. where both app and pages are working together at the same time. Otherwise there is likely to be conflicts and lost work if the parent branch moves pages and is merged later. I understand your point re intl being difficult to implement for both /pages and /app though! I found this example setup for this case here which may be worth trying first šŸ¤ž If possible, it'd be better to merge the parent branch asap.

Thanks so much and look forward to seeing this working!

boodland commented 2 months ago

Hi @annarhughes, no problem, very happy to contribute with the project.

I have already followed the example you mention as well as other that combines i18n with and without routing for App router without any success for the moment. Those are toy examples with no dependencies or complexity (no dynamic routing) and that uses routing approach ([locale] dynamic route) which force us to move almost everything within pages into the [locale] dynamic route with the consequent overhead.

I am going to start the parent PR with the sub PRs leaving the i18n migration for the last before the pages migration. I will work in the i18n migration sub PR to get it working for both router approaches, for the app one the implementation is straight forward and works smoothly, if I can't get it working for the Pages router we could consider the following solution:

  1. Merge the parent PR with dev.
  2. Create E2E tests for all the pages, one PR per page
  3. Create another Parent PR to hold all pages migration with a sub PR per page, a basic page migration (meet-the-team, chat, partnership, ...) takes around 30' each, the more complex I would say one hour, I could do it in 2-3 days, during that time we have to keep an eye on any changes in dev branch and merge them accordingly within the parent PR but I would say will be feasible and all the E2E tests would check if the migrated pages work as expected

Let's get the migration done šŸ’Ŗ . Let me know your thoughts on it.

annarhughes commented 2 months ago

@boodland I like these suggestions, the 2 parent PRs sounds like a good approach too šŸŒŸ i'm also hoping the pages don't take too long, and if its 2-3 days we can hold off all other merges for that time, no problem! I'll aim to review these PRs asap to help unblock and for branch management, but I generally work 2 days between mon-weds so there may be delays later in the week! Let me know if you're having any issues or want to discuss any decisions šŸ‘

boodland commented 2 months ago

@annarhughes perfect then. I am in the middle of a selection process for a position so maybe I won't be able to do any work until next week or later but I will take into account your availability and organise myself to do the work when can be reviewed.

eleanorreem commented 1 month ago

@boodland Anna is off for a bit so i will start picking this up.

Here is a new parent branch to work off next-js-migration