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

Upgrade starters to App Router #601

Closed
JohnAlbin opened this issue Nov 20, 2023 · 8 comments
Closed

Upgrade starters to App Router #601

JohnAlbin opened this issue Nov 20, 2023 · 8 comments

Comments

@JohnAlbin
Copy link
Collaborator

Package

basic-starter

Describe the feature request

The current starters both use the Pages directory for routing.

Describe the solution you'd like

We should clone the basic-starter to pages-starter. And clone graphql-starter to pages-graphql-starter.

And then we should upgrade the old starters to use the App Router.

@paolomainardi
Copy link

Thank you for providing this project, it is greatly appreciated.

Currently, I am exploring the possibility of converting the starter to the app router using the latest library, specifically the one found at https://www.npmjs.com/package/next-drupal/v/0.0.0-pr.600.128d03bc.

My objective at the moment is to gain a better understanding of how to migrate an existing project.

So far everything is working fine, however, I believe I may be missing something important.

Several functions utilize the context object, such as getResourceFromContext, which originates from the pages router, as seen here: https://github.com/chapter-three/next-drupal/blob/main/packages/next-drupal/src/get-resource.ts#L17.

How should I proceed with the conversion process? Would it be necessary to create a custom context object?

@paolomainardi
Copy link

Another example of the context object is this one translatePathFromContext() which basically expects an object with the slug attribute: https://github.com/chapter-three/next-drupal/blob/main/packages/next-drupal/src/client.ts#L962

Trying to convert to App Router, where we don't have anymore the context object, the only way I found to convert this one https://github.com/chapter-three/next-drupal-basic-starter/blob/main/pages/%5B...slug%5D.tsx, is:

export default async function Page({ params }: any) {
  const resource = await getPage(params);
  return (
    <Layout>
      <Head>
        <title>{resource.title}</title>
        <meta name="description" content="A Next.js site powered by Drupal." />
      </Head>
      {resource.type === "node--page" && <NodeBasicPage node={resource} />}
      {resource.type === "node--article" && <NodeArticle node={resource} />}
    </Layout>
  );
}

async function getPage(args: any) {
  const path = await drupal.translatePathFromContext({ params: args });
  if (!path) {
    return {
      notFound: true,
    };
  }

  const type = path.jsonapi?.resourceName;

  let params = {};
  if (type === "node--article") {
    params = {
      include: "field_image,uid",
    };
  }
  const resource = await drupal.getResourceFromContext<DrupalNode>(path, args, {
    params,
  });

  // At this point, we know the path exists and it points to a resource.
  // If we receive an error, it means something went wrong on Drupal.
  // We throw an error to tell revalidation to skip this for now.
  // Revalidation can try again on next request.
  if (!resource) {
    throw new Error(`Failed to fetch resource: ${path.jsonapi?.individual}`);
  }
  return resource;
}

It mostly works, but yet, not sure if I'm (ab)using the API by manually crafting the context object.

@paolomainardi
Copy link

@JohnAlbin sorry for coming back on this. Can you help me to understand how the context object should be treated in the app router context ?

@JohnAlbin
Copy link
Collaborator Author

JohnAlbin commented Dec 7, 2023

The context object is what I'm struggling with at the moment as well.

Let's walk through the old pages/[...slug].tsx flow so we all (including me) understand it.

How the old pages/[...slug].tsx works

Next.js runs getStaticPaths

  1. getStaticPaths receives a context object:
    type GetStaticPathsContext = {
      locales?: string[]
      defaultLocale?: string
    }
  2. getStaticPaths calls drupal.getStaticPathsFromContext(RESOURCE_TYPES, context)
  3. getStaticPaths returns paths which is an array of strings representing a slug for each page.

Next.js runs getStaticProps per entry in the paths array.

  1. getStaticProps receives a slightly different context object:
    type GetStaticPropsContext = {
      params?: Params
      preview?: boolean
      previewData?: Preview
      draftMode?: boolean
      locale?: string
      locales?: string[]
      defaultLocale?: string
    }
    
    The context.params is one of the items in the paths array returned by getStaticPaths
  2. getStaticProps then calls drupal.translatePathFromContext(context)
    1. translatePathFromContext uses context.params?.slug RED FLAG slug is a convention, not a specific property name. If the name of this file was [...path], the param would be path not slug. Also, looking at Next.js docs, this param is supposed to be an array of strings (each part in a path like some/slug1/slug2), not a single string; but our drupal.getStaticPathsFromContext returns a single string and not a string[].
    2. translatePathFromContext returns path: DrupalTranslatedPath
  3. getStaticProps gets the entity type from path?.jsonapi?.resourceName so that it knows which fields it should be asking for when it requests the entity and which React component to use to render the entity.
  4. getStaticProps calls drupal.getResourceFromContext( path, context, { params }) where path is a DrupalTranslatedPath, context is a GetStaticPropsContext, and params is the list of fields, sorting, etc.
  5. getStaticProps returns the resource that is returned from the previous step.

For each call to getStaticProps, Next.js renders the NodePage component with the resource prop returned from that getStaticProps call.

How a new app/[...slug]/page.tsx should work

Next.js runs generateStaticParams

  1. generateStaticParams is given an options object that contains the params from any parent directory dynamic routes. See https://nextjs.org/docs/app/api-reference/functions/generate-static-params#parameters In the next-drupal starters, there's no parent dynamic route, so params is empty.

  2. generateStaticParams shouldn't call drupal.getStaticPathsFromContext(RESOURCE_TYPES, context) because there is no context object. We'll need a new method to use and one doesn't exist right now. But in the interim, we can call drupal.getStaticPathsFromContext(RESOURCE_TYPES,{})

  3. generateStaticParams returns an array of params objects where one of the param property names MUST match the filename if it's a dynamic route filename. examples:

    • If used is in [category].tsx, the returned data should match { category: string }[]
    • If used is in [...slug].tsx, the returned data should match { slug: string[] }[]

    That means we'll need to let the developer massage the data returned by drupal.getStaticPathsFromContext() (or what we replace it with).

Next.js has removed the old getStaticProps step. So all of the old sub-steps need to be moved to the next step.

Lastly, Next.js renders a page using the NodePage component for each entry in the params array returned by generateStaticParams

  1. NodePage will need to check if draft mode is on with draftMode().isEnabled and, if it is, then figure out how to determine which revision Drupal wants us to show (TBD: Cookies?).
  2. NodePage should call drupal.translatePath(params.slug[0]) to get the path: DrupalTranslatedPath
  3. NodePage gets the entity uuid from path.entity.uuid and the entity type from path?.jsonapi?.resourceName so that it knows which fields it should be asking for when it requests the entity and which React component to use to render the entity.
  4. NodePage calls drupal.getResource(type, uuid, { params }) with the path or uuid and the revision ID of the desired entity and params, which is the list of fields, sorting, etc.
  5. NodePage then finishes rendering the returned entity.

@paolomainardi
Copy link

@JohnAlbin, first of all, let me say thank you so much for your super detailed answers. Give me some time to connect all the dots, and hopefully, I can contribute back with some code too.

@paolomainardi
Copy link

paolomainardi commented Dec 7, 2023

@JohnAlbin I've created a diagram of the Pages Router flow. I would appreciate your feedback before I continue with the other flow.

You can find it here: https://excalidraw.com/#room=f28794c0ac45452180bf,1OOdKSnoZByreHn6SkkZhA

And here as an attachment, just remove .txt part of the extension.
next-drupal-diagram-1.excalidraw.txt

@paolomainardi
Copy link

I've just added to the diagram even the new app router workflow, as you've described, it is the same link and attaached.

next-drupal-diagram-2.excalidraw.txt

JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 12, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
JohnAlbin added a commit that referenced this issue Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants