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

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

Draft
wants to merge 53 commits into
base: develop
Choose a base branch
from

Conversation

boodland
Copy link
Contributor

@boodland boodland commented Jul 26, 2024

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:

  • Create Root Layout
  • Add New Relic app router configuration
  • Move font imports to global css
  • Add GoogleTagManager and Rollbar scripts
  • Move primary main colour to a constant so it can be used in a server component
  • Add OpenGraph metadata, unable to use new metadata app router object as it has a non standard key attribute that doesn't seems to be used anywhere in the codebase, we could create the OG from the metadata object if we can remove it
  • Add Material UI app router configuration
  • Add i18n (next-intl) app router configuration and make it compatible with pages router, forced to do some hacks to get it working due to the way pages router i18n is setup/implemented. All hacks have been implemented in the middleware file which will be remove after the migration. This hacks brings some temporal UX impact like forward and back buttons doesn't work in app router pages when navigating for the same URL but with different locale and all url needs to have language/locale prepended, included the former default one 'en', everything will be reverted to normal and the i18n setup much easier once all the pages migration is completed, should be quick to migrate the pages.
  • Create storeProvider so it can be used in server components but its creation differ from the pages route approach, checked the content of the store and doesn't seem to be working, at least in dev mode, as I logged in with a superadmin and isSuperAdmin value was false but I was able to access to the dashboard/admin page
  • Create AppLayout to hold all the client components of the layout
  • Make StoryBlock client components so they can be used in app router
  • Add metadata to root layout
  • Add app router not found page that redirects to pages' one

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

boodland added 30 commits July 19, 2024 13:27
So they can be loaded for pages and app router
createTheme can not be imported in a server component that is what has been moved
Copy link

vercel bot commented Aug 13, 2024

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
Copy link
Contributor

eleanorreem commented Aug 14, 2024

@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:

  • I think you might have missed that we already have a cookie module - js-cookie. It would be good to just have one. If you think react-cookie is better, it would be good to know your reasoning. We can then migrate everything over to that package and uninstall js-cookie.
  • The branch is failing on yarn build

@annarhughes can do an indepth review.

@annarhughes
Copy link
Member

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 🙏

@boodland
Copy link
Contributor Author

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
Copy link
Member

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!

@annarhughes annarhughes self-assigned this Aug 14, 2024
@boodland
Copy link
Contributor Author

boodland commented Aug 14, 2024

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
Copy link
Member

@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
Copy link
Contributor Author

@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.

@boodland boodland marked this pull request as draft August 15, 2024 10:09
@eleanorreem
Copy link
Contributor

eleanorreem commented Aug 23, 2024

@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

@eleanorreem
Copy link
Contributor

@boodland is this PR okay to be closed?

@boodland
Copy link
Contributor Author

@boodland is this PR okay to be closed?

Hi @eleanorreem, it is okay to close it but do not remove it as I am using it as reference for the subtasks.

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.

3 participants