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

[PROPOSAL] Next.js Templating #235

Closed
blakewilson opened this issue May 20, 2021 · 10 comments
Closed

[PROPOSAL] Next.js Templating #235

blakewilson opened this issue May 20, 2021 · 10 comments
Labels
help wanted package: next (legacy) Related to the legacy next package type: feature New functionality being added

Comments

@blakewilson
Copy link
Contributor

blakewilson commented May 20, 2021

NOTE: This is still a work in progress, feedback welcome.

Currently, @wpengine/headless provides templating functionality for use in Next.js. This includes the templates in wp-templates/, getNextStaticProps(), getNextStaticPaths(), etc.

With the improvements in #232, it will become much easier to query and fetch data.

With these improvements, we may want to consider leveraging the native Next.js page routing system, where the user would implement the data fetching functionality and getStaticProps/getStaticPaths themselves.

An example Next.js app using this format will look like:

pages/
├─ preview/ <- All pages under here are CSR/SSR only
│  ├─ post/
│  │  ├─ [[...postPreviewUri]].tsx
│  ├─ page/
│  │  ├─ [[...pagePreviewUri]].tsx
├─ posts/
│  ├─ index.tsx
│  ├─ [postSlug].tsx <-- /posts/hello-world
│  ├─ page/
│  │  ├─ [paginationTerm]/
│  │  │  ├─ [postCursor].tsx <-- posts/page/[before|after]/tbsda==
├─ category/
│  ├─ [categorySlug].tsx <-- category/featured
│  ├─ page/
│  │  │  ├─ [paginationTerm]/
│  │  │  |  ├─[categoryCursor].tsx <-- category/featured/[before|after]/tbsda==
├─ _app.tsx
├─ 404.tsx
├─ index.tsx
├─ about.tsx
├─ [...pageUri].tsx
@wjohnsto
Copy link
Contributor

wjohnsto commented May 21, 2021

I think you're right on this one. I believe in Next.js the wp-templates/ piece is more of an anti-pattern given that Next provides it's own router.

To expand on getNextStaticProps and getNextStaticPaths, and @apollo/client in general:

  • getNextStaticPaths does not add any value in it's current state, and perhaps it doesn't need to exist.
  • getNextStaticProps fetches data proactively, which does not work well for customization (see this hack I wrote to make sure we stringify the right query so we can restore it on the frontend.

The other thing we need to think about related to this: @apollo/client recommends extracting the query cache server-side and restoring it client-side so you can write more idiomatic React code. With Next pages you are instructed to return props from the associated SSR/SSG functions, and those props will be passed into your page. So normal Next code might look like this:

import React from 'react';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPosts } from '@wpengine/headless-core';

export default function Page({ posts }) {
  return <Posts posts={posts} />;
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  const posts = await getPosts(client);

  return {
    revalidate: 1,
    props: {
      posts,
    },
  };
}

A note on the code ☝️, there is no opportunity there for us to put the Apollo cache on props or otherwise return a 404. Perhaps we could have something like the following to take care of the Apollo cache:

import React from 'react';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, addApolloClientCacheToProps, getPosts } from '@wpengine/headless-core';

export default function Page({ posts }) {
  return <Posts posts={posts} />;
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  const posts = await getPosts(client);

  const props = addApolloClientCacheToProps({
    posts,
  });

  return {
    revalidate: 1,
    props,
  };
}

With @apollo/client caching thrown into the mix it might look more like this (if we don't proactively fetch):

import React from 'react';
import {
  getNextStaticProps,
  usePost,
} from '@wpengine/headless-next';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPosts } from '@wpengine/headless-core';

export default function Page() {
  const posts = usePosts();
  return <Posts posts={posts} />;
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  await getPosts(client);

  return getNextStaticProps(context);
}

The second example seems a little cleaner and supports optional client-side requests where necessary as well as the ability to add in SSG/SSR without having to reconfigure your application. However, the second example does involve a little bit of indirection because the app developer has to understand that the framework is going to cache the Apollo client and restore it with the HeadlessProvider client side. This also can get confusing when it comes to wanting to provide your own props, return 404/error pages, etc. I don't like the current way to do this:

import React from 'react';
import {
  getNextStaticProps,
  usePost,
} from '@wpengine/headless-next';
import Head from 'next/head';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPosts } from '@wpengine/headless-core';

export default function Page({ title }) {
  const posts = usePosts();
  return (<>
    <Head>
      <title>{title}</title>
    </Head>
    <Posts posts={posts} />
  </>);
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  await getPosts(client);

  const result = getNextStaticProps(context);
  result.props.title = 'Posts'; // will break on a 404
  
  return result;
}

A better way to do the above logic might be:

import React from 'react';
import {
  getNextStaticProps,
  usePost,
} from '@wpengine/headless-next';
import Head from 'next/head';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPosts } from '@wpengine/headless-core';

export default function Page({ title }) {
  const posts = usePosts();
  return (<>
    <Head>
      <title>{title}</title>
    </Head>
    <Posts posts={posts} />
  </>);
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  await getPosts(client);

  return getNextStaticProps(context, {
    props: {
      title: 'Posts',
    }
  });
}

The code above would still support us returning a 404, the question is should we be responsible for determining a 404? We can certainly provide logic that attempts to inform on 404 pages via nodeByUri, but it might be wrong to assume that if is404: true then the app definitely wants to return a 404.

@matt-landers
Copy link
Contributor

Intuition says to return the props from getStaticProps and not use the hook in the component. When navigating client-side, Next will still handle passing the props to the component. This allows you to create idiomatic code to Next for a Next project. If you were to have a page that was purely client-side routed, you could use the hook in the component. We should still cache the query on the client when possible in the case it used elsewhere.

For 404s, I think we should default to handling them but allow a away to opt-out.

@wjohnsto
Copy link
Contributor

wjohnsto commented May 21, 2021

Intuition says to return the props from getStaticProps and not use the hook in the component. When navigating client-side, Next will still handle passing the props to the component. This allows you to create idiomatic code to Next for a Next project. If you were to have a page that was purely client-side routed, you could use the hook in the component. We should still cache the query on the client when possible in the case it used elsewhere.

For 404s, I think we should default to handling them but allow a away to opt-out.

Doing all of this (apollo client cache, 404 handling, prop passing) would require some interface similar to above, but could end with code similar to this:

import React from 'react';
import {
  getNextStaticProps,
  usePost,
} from '@wpengine/headless-next';
import Head from 'next/head';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPosts } from '@wpengine/headless-core';

export default function Page({ title, posts }) {
  // Not needed, but could also work if you want to do it this way
  // const posts = usePosts();
  
  return (<>
    <Head>
      <title>{title}</title>
    </Head>
    <Posts posts={posts} />
  </>);
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  const posts = await getPosts(client);

  // We probably want to avoid combining the handle404 configuration with the GetStaticPropsResult to extend and return.
  // Next yells at you when you have invalid properties.
  return getNextStaticProps(context, {
    handle404: false,
  }, {
    props: {
      title: 'Posts',
      posts,
    }
  });
}

The 404 handling is tricky since the only way we know is based on the nodeByUri response, which means we must find a way to respect the WordPress URL scheme.

@matt-landers
Copy link
Contributor

Agreed on the 404. We need to think that through a bit more, but I like this example better. It looks more like Next. I don't want you to have to reconcile between Next docs and ours. You should be able to follow most of Next's conventions without incompatibilities in what the framework expects.

@blakewilson
Copy link
Contributor Author

blakewilson commented May 21, 2021

The 404 handling is tricky since the only way we know is based on the nodeByUri response, which means we must find a way to respect the WordPress URL scheme.

Another consideration if we are going to follow Next's conventions of pages/routing, is how we will infer the URL from getStaticProps to be used by nodeByUri.

In the case of a nested route, pages/category/[...categorySlug].tsx for example, the params of GetStaticPropsContext do not necessarily reflect the URL path:

// pages/category/[...categorySlug].tsx

export default function Page() {
  ...
}

export async function getStaticProps(context: GetStaticPropsContext) {
  /**
   * the expected URL would be "/category/uncategorized".
   * params does not indicate the "category" path part.
   */
  console.log(context?.params) <- "{ categorySlug: [ 'uncategorized' ] }"
}

We may have to infer the URL based on a combination of GetStaticPropsContext params and something like the __filename, if that would even be possible.

@wjohnsto
Copy link
Contributor

wjohnsto commented May 21, 2021

NOTE: __filename will always end up being /index.js because of how Next routing works I believe.

Good point, with GetServerSidePropsContext we can use context.resolvedUrl but with GetStaticProps we only have access to context.params, and we can't be sure what keys to use and how to issue a query based on them. @wpengine/headless currently assumes you define your page with [[...page]].tsx, and perhaps we can have a convention for each type of node that you might want to request. Something like this:

pages/
├─ preview/ <- All pages under here are CSR/SSR only
│  ├─ post/
│  │  ├─ [[...postPreview]].tsx
│  ├─ page/
│  │  ├─ [[...pagePreview]].tsx
├─ posts/
│  ├─ [post].tsx
├─ category/
│  ├─ [category].tsx
├─ [page]/
│  ├─ [[...pageUri]].tsx
├─ _app.tsx
├─ 404.tsx
├─ index.tsx

I think the above works, and we would look for a params object to look like one of the following:

// /preview/post?p=197&preview=true
params = {
  postPreview: [],
};

// /preview/page?page_id=3&preview=true&p=3
params = {
  pagePreview: [],
};

// /posts/hello-world
params = {
  post: 'hello-world',
};

// /category/uncategorized
params = {
  category: 'uncategorized',
};

// /about
params = {
  page: 'about',
  pageUri: [],
};

// /pricing/startup
params = {
  page: 'pricing',
  pageUri: ['startup']
};

There is another possibility for someone to want to have both posts and pages under the root URL. So you might have /about be a page and /hello-world be a post. In this case you would need something like:

pages/
├─ preview/ <- All pages under here are CSR/SSR only
│  ├─ post/
│  │  ├─ [[...postPreview]].tsx
│  ├─ page/
│  │  ├─ [[...pagePreview]].tsx
├─ category/
│  ├─ [category].tsx
├─ [[...postOrPageUri]].tsx
├─ _app.tsx
├─ 404.tsx
├─ index.tsx

This would be a more difficult case to target, an I'm not sure how often it comes up. We could certainly use nodeByUri to know if the URI is a post or a page, but most of that logic would have to be done by the app developer. I think we should not spend a lot of time supporting this scenario unless it is incredibly common. It needs to be possible, but there might be some extra code required on the part of the application.

@blakewilson
Copy link
Contributor Author

NOTE: __filename will always end up being /index.js because of how Next routing works I believe.

@wjohnsto I figured something like that was the case. I played around with it a little, and it looks like it may be possible, the __filename usually returns something like .next/server/pages/....

With that being said, your convention works much better, particularity around the structure of params. I think this will handle most use cases.

I do think that the posts and pages being under the root URL will come up often. In fact, this happened yesterday while I was building a POC using this new proposal. I think this is just something that we'll have to provide a guide for.

@wjohnsto
Copy link
Contributor

I think if you want to use the root URL-based page and post routes then you will need to use our getUriInfo or useUriInfo to inform what to render. Something like this:

// pages/[[...postOrPageUri]].tsx

import React from 'react';
import {
  getNextStaticProps,
} from '@wpengine/headless-next';
import { GetStaticPropsContext } from 'next';
import { getApolloClient, getPost, getPage, getUriInfo } from '@wpengine/headless-core';
import { Post, Page } from '../components';

export default function PostPage({ post, page }) {
  return (<>
    { post && <Post post={post} /> }
    { page && <Page page={page} /> }
  </>);
}

export async function getStaticProps(context: GetStaticPropsContext) {
  const client = getApolloClient(context);
  const uriInfo = await getUriInfo(client, Array.from(context?.params?.postOrPage).join('/'));
  let post: WPGraphQL.Post | null = null;
  let page: WPGraphQL.Page | null = null;

  if (uriInfo.isPage) {
    page = await getPage(client);
  } else {
    post = await getPost(client);
  }

  // We probably want to avoid combining the handle404 configuration with the GetStaticPropsResult to extend and return.
  // Next yells at you when you have invalid properties.
  return getNextStaticProps(context, {
    props: {
      post,
      page,
    }
  });
}

@blakewilson
Copy link
Contributor Author

blakewilson commented May 24, 2021

Adding this feature here as well to keep track of it. We may have users that need multi-page posts. I don't think this functionality is very common, but it may come up.

In this case, we could have a guide to support this. To achieve this we would have to alter the posts directory to look similar to:

pages/
├─ posts/
│  ├─ [post]/
│  │  ├─ index.js
│  │  ├─ page/
│  │  │  ├─ [pageNumber].js

With params that look like:

// /posts/hello-world
params = {
  post: 'hello-world',
};

// /posts/hello-world/page/2
params = {
  post: 'hello-world',
  pageNumber: '2'
};

@blakewilson
Copy link
Contributor Author

This was resolved in #270

@josephfusco josephfusco added type: feature New functionality being added and removed proposal labels Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted package: next (legacy) Related to the legacy next package type: feature New functionality being added
Projects
None yet
Development

No branches or pull requests

4 participants