department-of-veterans-affairs / va.gov-cms

Editor-centered management for Veteran-centered content.
https://prod.cms.va.gov
GNU General Public License v2.0
99 stars 70 forks source link

Implement App Router #14801

Open timcosgrove opened 11 months ago

timcosgrove commented 11 months ago

User Story or Problem Statement

We should convert from our current page router-based setup to the new app router paradigm, as outlined in #14474 .

## Acceptance Criteria
- [ ] Route handling is changed from Page router to App router style
- [ ] A static build completes successfully

Description or Additional Context

A general migration guide is available in the Next.js documentation

Steps for Implementation

Initially here. Below, these steps are expanded more granularly.

### General tasks
- [ ] [Upgrade ESLint version](https://nextjs.org/docs/pages/building-your-application/upgrading/app-router-migration#eslint-version)
### `src/pages/[[...slug]].tsx`
- [ ] Move to `src/app/[[...slug]]/page.tsx`
- [ ] Replace `getStaticPaths` with `generateStaticParams`
- [ ] Remove `getStaticProps` and move functionality into component body
- [ ] Replace `return { notFound: true}` with `notFound()`
### `src/pages/_playground/api-explorer.tsx`
- [ ] Move to `src/app/_playground/api-explorer/page.tsx`
- [ ] Remove `getStaticProps` and move functionality into component body
- [ ] Replace `return { notFound: true}` with `notFound()`
### `src/pages/api/exit-preview.tsx`
- [ ] Move to `src/app/api/exit-preview/route.ts`
- [ ] Rename `exit` function `GET`
### `src/pages/api/preview.tsx`
- [ ] Move to `src/app/api/preview/route.ts`
### `src/pages/_app.tsx` and `src/pages/_document.tsx`
- [ ] Combine and move to `src/app/layout.tsx`
timcosgrove commented 11 months ago

If the preview routes present any difficulties, they can be deferred. We will have separate epic/tickets for preview.

ryguyk commented 11 months ago

Upon deeper investigation, next-drupal seems to not support App Router.

next-drupal provides functions getStaticPathsFromContext and translatePathFromContext, which require the context parameter that is passed to getStaticPaths and getStaticProps (in Pages Router). Immediately, I see two obstacles trying to get these functions provided by next-drupal to work with App Router:

  1. The context parameter is not passed to generateStaticParams (App Router).
  2. The function getStaticPathsFromContext is raising errors being called from generateStaticParams.
ryguyk commented 11 months ago

The function getStaticPathsFromContext is raising errors being called from generateStaticParams.

The problem is with the usage of DrupalClient. More specifically, it seems to be due to the fact that next-drupal packages DrupalClient together with all other functionality, specifically client-only functionality, so importing DrupalClient is raising an error when that import happens in an page defined as a Server Component using App Router.

It seems like Webpack should be able to handle this and tree shake things out, but it's seemingly tripping over things. The client-only code does seem to be identified as unused code, but there's a TypeError being raised in one of the generated chunk files.

I'm still investigating this.

ryguyk commented 11 months ago

I forked next-drupal and configured things to package DrupalClient separately from the rest of the code. This was very much just a test, but it did work to resolve the issue described in the previous comment.

ryguyk commented 11 months ago

Another possible path forward is to migrate to App Router but to keep the pages as Client Components ("use client"). This is effectively what Pages Router does by default, so we wouldn't be losing anything on that front.

This problem would remain:

The context parameter is not passed to generateStaticParams (App Router).

next-drupal doesn't provide App-Router equivalents to getStaticPathsFromContext and translatePathFromContext, but there might be a workaround. It seems possible that we can still use those methods and construct the context object manually. Initially, at least, this could likely be an empty object when passed to getStaticPathsFromContext. For translatePathsFromContext, we'd construct it from the path parameters passed to the component from generateStaticParams.

ryguyk commented 11 months ago

Summary on possible paths forward

1. Stay on Pages Router (temporarily or permanently)

2. Move to App Router but use Client Components

3. Move to App Router and fork next-drupal for our needs

4. Move to App Router and implement custom alternative to DrupalClient

5. Try to enable move to App Router by investigating configuration changes to prevent errors in Webpack chunk files.

alexfinnarn commented 8 months ago

Looks like there is movement in next-drupal to support App Router:

Moves the use-menu hook to a new entry point so that developers can import any of the other exports from the main entry point into their Server Components.

So, maybe moving to App Router without forking next-drupal will be possible by the time this issue is reviewed again.