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

meta: adopt next-intl and app router #6092

Merged
merged 2 commits into from
Nov 7, 2023
Merged

Conversation

araujogui
Copy link
Member

@araujogui araujogui commented Nov 3, 2023

Description

This PR refactors the Node.js Website and adopts the new Next.js App Router. This PR does all the necessary changes to make the app router-compatible. Bear in mind that some changes here are temporary.

Note: This PR is still going to have some updates, such as improvement of documentation, updates on Collaborator Guides, etc.

Changes

  • Removed a bunch of React Providers of our "static data" (such as blog data) and make them available directly on Hooks
  • Removed react-intl and our custom i18n middleware in favour of next-intl
  • Refactored our i18n keys to follow the ICU format that next-intl uses
  • Updated several components to use next-intl and its components
  • Removed LocalizedLink, useLocale, etc in favor of next-intl builtin's
  • Refactored next.dynamic.mjs, our App Root Layout, and all the core app/ folders to make the whole process of building and rendering compatible.
  • Other minor changes that are related to make this compatible
  • Removed HTMLHead and added App Router Metadata generation
  • Moved Node Release Data Parsing directly to Release Data Generation
  • Removed unused types, utilities and Hooks
  • Tried to simplify and optimize existing Layouts and removed duplicated Layouts
  • Overall cleanup of Application logic
  • Updated Unit Tests to make them work
  • Fixed imports

Caveat

This rework is stable, but next-intl 3.0 is still on Release Candidate, and a few things might change. For example, the usage of an unstable API https://next-intl-docs-git-feat-next-13-rsc-next-intl.vercel.app/docs/getting-started/app-router#add-unstable_setrequestlocale-to-all-layouts-and-pages

Validation

Builds should pass, Linting should pass, Deployment should pass, all pages should work as expected.

Deep testing is needed here!

Copy link

vercel bot commented Nov 3, 2023

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

Name Status Preview Comments Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 7, 2023 6:17pm

@ovflowd ovflowd changed the title Refactor/i18n meta: adopt next-intl and app router Nov 3, 2023
@ovflowd ovflowd marked this pull request as ready for review November 3, 2023 12:14
@ovflowd ovflowd requested review from a team as code owners November 3, 2023 12:14
@ovflowd ovflowd marked this pull request as draft November 3, 2023 12:29
@ovflowd ovflowd added enhancement i18n Issues/PRs related to the Website Internationalisation infrastructure Issues/PRs related to the Repository Infra meta Meta Issues for Administration of the Website Team github_actions:pull-request Trigger Pull Request Checks labels Nov 3, 2023
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Nov 3, 2023
Copy link

github-actions bot commented Nov 3, 2023

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 93 🟢 97 🟢 92 🟢 92 🔗
/en/about 🟢 100 🟢 95 🟢 92 🟢 92 🔗
/en/about/previous-releases 🟢 100 🟢 96 🟢 92 🟢 92 🔗
/en/download 🟢 100 🟢 97 🟢 92 🟢 92 🔗
/en/blog 🟢 98 🟢 97 🟢 92 🟢 92 🔗

Copy link

github-actions bot commented Nov 3, 2023

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 84%
79.1% (352/445) 63.46% (66/104) 62.22% (56/90)

Unit Test Report

Tests Skipped Failures Errors Time
54 0 💤 0 ❌ 0 🔥 5.146s ⏱️

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Nov 3, 2023
@ovflowd ovflowd marked this pull request as ready for review November 3, 2023 19:35
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Nov 3, 2023
@ovflowd
Copy link
Member

ovflowd commented Nov 7, 2023

Hey y'all, as I believe that a full review of this PR is pretty hard, and to unblock further development. I'm queueing this to our merge queue.

Please feel free to do any sort of reviews post-merge. I've battle-tested this and so far all good, so let's do it.

@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 7, 2023
code: kept working on adoption of i18n

feat: app router transition

meta: renamed sections to containers due to fs casing

fix: force static

fix: minor fixes of build and caching

feat: metadata of each page

fix: tests and stuff

fix: tiny type fix

chore: updated link to guide

chore: intl on stories

chore: no usage of assert

refactor: some more cleanups on codebase

meta: some improvements and fixes

refactor: more code review changes

refactor: more standardisation next-intl

chore: minor changes to 404

refactor: code cleanup and reusability

fix: fixed tests and imports

chore: minor fixes and test updates

fix: sitemap generation and 404 ignore

chore: cleanup activelocalizedlink

chore: remove legacy sto-top

chore: more cleanups

chore: more cleanups

chore: use right import

chore: optimise generation of blog meta

chore: small cleanup

chore: updated collaborator guide

meta: provide server and client version of hooks

fix: tests imports

fix: styles of blogcard
@ovflowd
Copy link
Member

ovflowd commented Nov 7, 2023

cc @nodejs/web-infra regarding my message above ☝️

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Nov 7, 2023
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Nov 7, 2023
@ovflowd ovflowd added this pull request to the merge queue Nov 7, 2023
Merged via the queue into nodejs:main with commit b511479 Nov 7, 2023
14 of 15 checks passed
Copy link
Member

@canerakdas canerakdas left a comment

Choose a reason for hiding this comment

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

I have only minor review notes ✨ In addition to these changes, it would be good to update the part of the collaborator guide that points to the location of the layouts (https://github.com/nodejs/nodejs.org/blob/main/COLLABORATOR_GUIDE.md?plain=1#L143-L145)

Comment on lines +22 to +23
export const generateMetadata = async (c: DynamicParams) => {
const { path = [], locale = defaultLocale.code } = c.params;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const generateMetadata = async (c: DynamicParams) => {
const { path = [], locale = defaultLocale.code } = c.params;
export const generateMetadata = async ({params} : DynamicParams) => {
const { path = [], locale = defaultLocale.code } = params;

c doesn't mean much, we can deconstruct directly

Comment on lines +20 to 30
for (const locale of availableLocaleCodes) {
const routesForLanguage = await dynamicRouter.getRoutesByLanguage(locale);

paths.push(...routesForLanguage.map(route => `${locale}/${route}`));
}

// The current date of this request
const currentDate = new Date().toISOString();

const appRoutes = [...dynamicRoutes, ...staticPaths]
.sort()
.map(route => `${baseUrlAndPath}/${route}`);
const appRoutes = paths.sort().map(route => `${baseUrlAndPath}/${route}`);

return [...appRoutes, ...EXTERNAL_LINKS_SITEMAP].map(route => ({
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for (const locale of availableLocaleCodes) {
const routesForLanguage = await dynamicRouter.getRoutesByLanguage(locale);
paths.push(...routesForLanguage.map(route => `${locale}/${route}`));
}
// The current date of this request
const currentDate = new Date().toISOString();
const appRoutes = [...dynamicRoutes, ...staticPaths]
.sort()
.map(route => `${baseUrlAndPath}/${route}`);
const appRoutes = paths.sort().map(route => `${baseUrlAndPath}/${route}`);
return [...appRoutes, ...EXTERNAL_LINKS_SITEMAP].map(route => ({
for (const locale of availableLocaleCodes) {
const routesForLanguage = await dynamicRouter.getRoutesByLanguage(locale);
paths.push(...routesForLanguage.map(route => `${baseUrlAndPath}/${locale}/${route}`));
}
const currentDate = new Date().toISOString();
return [...paths, ...EXTERNAL_LINKS_SITEMAP].map(route => ({

We can directly pass baseUrlAndPath no need to sort and map again

@@ -0,0 +1,24 @@
import { cache } from 'react';

import type { ClientSharedServerContext } from './types';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import type { ClientSharedServerContext } from './types';
import type { ClientSharedServerContext } from '@/types';

@ovflowd
Copy link
Member

ovflowd commented Nov 8, 2023

I have only minor review notes ✨ In addition to these changes, it would be good to update the part of the collaborator guide that points to the location of the layouts (main/COLLABORATOR_GUIDE.md?plain=1#L143-L145)

@canerakdas can you open a new PR with these changes? (All of them)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement i18n Issues/PRs related to the Website Internationalisation infrastructure Issues/PRs related to the Repository Infra meta Meta Issues for Administration of the Website Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants