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

Implement App Router #14801

Open
14 tasks
timcosgrove opened this issue Aug 16, 2023 · 7 comments
Open
14 tasks

Implement App Router #14801

timcosgrove opened this issue Aug 16, 2023 · 7 comments
Assignees
Labels
Accelerated Publishing Needs refining Issue status Post MVP To be completed after MVP

Comments

@timcosgrove
Copy link
Contributor

timcosgrove commented Aug 16, 2023

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

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

src/pages/[[...slug]].tsx

src/pages/_playground/api-explorer.tsx

src/pages/api/exit-preview.tsx

src/pages/api/preview.tsx

src/pages/_app.tsx and src/pages/_document.tsx

@timcosgrove
Copy link
Contributor Author

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

@ryguyk
Copy link
Contributor

ryguyk commented Aug 18, 2023

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

ryguyk commented Aug 24, 2023

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

ryguyk commented Aug 24, 2023

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

ryguyk commented Aug 24, 2023

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

ryguyk commented Aug 24, 2023

Summary on possible paths forward

1. Stay on Pages Router (temporarily or permanently)

  • This would amount to doing nothing. We're on Pages Router currently.
  • We could wait for next-drupal support to migrate to App Router.

2. Move to App Router but use Client Components

  • THIS MIGHT NOT BE A SIMPLE SOLUTION. Client components cannot be async and that means I'm not immediately sure how we'd fetch the data.
  • This is what Pages Router does by default, so we wouldn't be losing anything in terms of functionality, but we wouldn't really be gaining any of the benefits of App Router either.
  • Although we wouldn't gain much immediately, this would likely situate us better for full cutover in the future. This could perhaps be when next-drupal updates for App-Router support (or if we decide we do have to move partially or completely off of next-drupal).
  • This would require some Webpack configuration:
  webpack: (config, { isServer }) => {
    if (!isServer) {
        // don't resolve these server-only modules on the client
        config.resolve.fallback = {
            tls: false,
            fs: false,
            net: false,
            dns: false,
        }
    }

    return config;
  },

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

  • I worked on a POC that successfully split out DrupalClient from other components and that removed the client-code-in-a-server-component problem described above, which is the main blocker currently. It was a very rough POC, though, and would require some more work.
  • Forking would come with all the downsides it always comes with. We'd effectively be maintaining our own version of the library.

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

  • This would likely not be a complete removal of next-drupal but it would almost amount to that. It probably could amount to that, but we can probably still benefit from some type definitions and maybe a few other things.
  • Engineering the custom alternative would probably not be a light lift.

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

  • It seems like the client-only code from next-drupal should not be called/bundled/accessed in any way, yet it is causing problems. We can investigate whether things can be configured differently to resolve this.

@timcosgrove timcosgrove added Post MVP To be completed after MVP Needs refining Issue status labels Aug 29, 2023
@alexfinnarn
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accelerated Publishing Needs refining Issue status Post MVP To be completed after MVP
Projects
None yet
Development

No branches or pull requests

3 participants