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 Next.js v14 #624

Merged
merged 2 commits into from
Dec 13, 2023
Merged

Upgrade starters to Next.js v14 #624

merged 2 commits into from
Dec 13, 2023

Conversation

JohnAlbin
Copy link
Collaborator

This pull request is for:

  • examples/*
  • modules/next
  • packages/next-drupal
  • starters/basic-starter
  • starters/graphql-starter
  • Other

GitHub Issue: #601

Describe your changes

Upgrading the starters to use the App Router is going to involve a lot of changes, including adding Draft mode and possibly internationalization (with the removal of the context object).

To make it easier to review all the changes, this PR simply upgrades the starters to use the latest APIs and type definitions of Next.js v14 while still using the pages router. It's the first step to fully upgrade the starters.

Copy link

vercel bot commented Dec 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
next-drupal ⬜️ Ignored (Inspect) Visit Preview Dec 13, 2023 5:56pm

@JohnAlbin JohnAlbin requested a review from robdecker December 12, 2023 17:54
@JohnAlbin JohnAlbin force-pushed the 601-starters-v14 branch 3 times, most recently from 4ba9957 to d01d282 Compare December 13, 2023 11:49
Copy link
Collaborator Author

@JohnAlbin JohnAlbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've annotated my commits to make it easier to understand the work while reviewing the "Files changed".

# Starters should never have a lock file.
package-lock.json
pnpm-lock.yaml
yarn.lock
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By putting this in the root of starters/ directory, it prevents any lock files in any starter we ever have.

DRUPAL_REVALIDATE_SECRET=Retrieve this from /admin/config/services/next
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated this to match the format of the "Environment variables" admin page at admin/config/services/next/sites/[site]/env

I should note that DRUPAL_PREVIEW_SECRET isn't actually used by any code on the Next.js side. But we can look at that in a separate issue.

@@ -1,4 +1,4 @@
{
"extends": "next",
"extends": "next/core-web-vitals",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new (more strict) ESLint config used by default with create-next-app

# typescript
*.tsbuildinfo
next-env.d.ts
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was a copy of the .gitignore file generated by old versions of the create-next-app. I've updated this to use the new copy from the most recent create-next-app.

But I also added the "IDE files" section because I ❤️ PHPStorm and always have to add it the projects I work on.

{
"semi": false,
"trailingComma": "es5"
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added Prettier to the starters because I had issues running the starters in-place in the monorepo. And it was safer to copy the whole directory into a new place for development. But then it was difficult to ensure I was modifying the files that was still compatible with our code styles.

By adding Prettier to the starters, this gives the added feature to any user who doesn't like this code style (*cough* like me *cough*) that they can now easily change .prettierrc.json to use a code style that they do like and they can use the new "format" package script to re-write all the files to that new format in one easy step.

@@ -1,2180 +0,0 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't add a yarn.lock file to a starter that is supposed to get the most up-to-date dependencies when a developer starts a new project.

@@ -1,10 +1,15 @@
# See https://next-drupal.org/docs/environment-variables
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes to graphql-starter are almost identical to the changes in the basic-starter.

onClick={() => router.push("/api/exit-preview")}
>
Exit preview mode
</button>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In preview-alert.tsx this was:

{/* eslint-disable @next/next/no-html-link-for-pages */}
<a href="/api/exit-preview" className="text-white underline">
  Click here
</a>{" "}
to exit preview mode.

But the semantics of a link are "take me to a new webpage" and that's not what that does. It's a button because clicking on it performs an action and not a location change. And don't get me started on "click here"!

This is a common accessibility issue, so I've changed element to a button.

height={480}
alt={node.title}
/>
<Image src={node.image.url} width={768} height={480} alt="" />
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The node title is not the alt text for its image. But I don't see that data in the GraphQL explorer, so an empty alt text is a more accessible option.

if (!context?.params?.slug) {
return {
notFound: true,
}
}

const data = await query<{
nodeByPath: Article
nodeByPath: NodeArticle | NodePage
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was actually a bug in the previous type definition. It should be an OR.

@JohnAlbin
Copy link
Collaborator Author

lol. Those 3 tests are failing again. Now with the opposite HTTP response code as the PR that changed them. :-p

getResource › it throws an error if revision access is forbidden

Expected substring: "401 Unauthorized
No authentication credentials provided."

Received message:   "403 Forbidden
The current user is not allowed to GET the selected resource. The user does not have access to the requested version."

@JohnAlbin JohnAlbin force-pushed the 601-starters-v14 branch 2 times, most recently from 5587d22 to 13d8ea5 Compare December 13, 2023 16:48
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent, this should be Layout.tsx

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I didn't notice this on my Mac because the filename is Layout.tsx but git doesn't recognize the case change on a case-sensitive file system.

Copy link
Member

@robdecker robdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just the one file name change, then merge it.

@JohnAlbin JohnAlbin force-pushed the 601-starters-v14 branch 2 times, most recently from 7446125 to 7fd4e80 Compare December 13, 2023 17:53
Copy link
Member

@robdecker robdecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@JohnAlbin JohnAlbin merged commit 3561226 into main Dec 13, 2023
9 checks passed
@JohnAlbin JohnAlbin deleted the 601-starters-v14 branch December 13, 2023 18:42
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.

2 participants