diff --git a/.github/workflows/local-dev.yml b/.github/workflows/local-dev.yml index fb5f305047..67e832e6e3 100644 --- a/.github/workflows/local-dev.yml +++ b/.github/workflows/local-dev.yml @@ -21,7 +21,6 @@ jobs: - name: Check out repo uses: actions/checkout@3df4ab11eba7bda6032a0b82a6bb43b11571feac # v4.0.0 - # - uses: ./.github/actions/node-npm-setup - name: Install dependencies run: npm install @@ -31,6 +30,20 @@ jobs: - name: Disable Next.js telemetry run: npx next telemetry disable + # The Playwright test, with the env vars we set here, takes care of + # starting a server and shutting it down when it's done. + # That's why it's important this step comes before the `npm start &` + # step below. + - name: Run Playwright tests + env: + # This is what local dev contributors are expected to do. + PLAYWRIGHT_START_SERVER_COMMAND: 'npm start' + # This is so that timeouts aren't retried, which can lead to + # tests not exiting at the end with a non-zero. Otherwise, + # by default failures are marked as "flaky" instead of "failed". + PLAYWRIGHT_RETRIES: 0 + run: npm run playwright-test -- playwright-local-dev + - name: Start server in the background run: npm start > /tmp/stdout.log 2> /tmp/stderr.log & diff --git a/README.md b/README.md index d936faade4..61a7458c67 100644 --- a/README.md +++ b/README.md @@ -4,23 +4,23 @@ This repository contains the documentation website code and Markdown source file GitHub's Docs team works on pre-production content in a private repo that regularly syncs with this public repo. -Use the table of contents icon on the top left corner of this document to navigate to a specific section quickly. +Use the table of contents icon Table of contents icon on the top left corner of this document to navigate to a specific section quickly. ## Contributing We accept different types of contributions, including some that don't require you to write a single line of code. For detailed instructions on how to get started with our project, see "[About contributing to GitHub Docs](https://docs.github.com/en/contributing/collaborating-on-github-docs/about-contributing-to-github-docs)." - ### Ways to contribute + On the GitHub Docs site, you can contribute by clicking the **Make a contribution** button at the bottom of the page to open a pull request for quick fixes like typos, updates, or link fixes. You can also contribute by creating a local environment or opening a Codespace. For more information, see "[Setting up your environment to work on GitHub Docs](https://docs.github.com/en/contributing/setting-up-your-environment-to-work-on-github-docs)." - +Contribution call-to-action For more complex contributions, please open an issue using the most appropriate [issue template](https://github.com/github/docs/issues/new/choose) to describe the changes you'd like to see. -If you're looking for a way to contribute, you can scan through our [help wanted board](https://github.com/github/docs/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22) to find open issues already approved for work. +If you're looking for a way to contribute, you can scan through our [help wanted board](https://github.com/github/docs/issues?q=is%3Aopen+is%3Aissue+label%3A%22help+wanted%22) to find open issues already approved for work. ### Join us in discussions diff --git a/src/fixtures/playwright.config.ts b/src/fixtures/playwright.config.ts index fa5c3771f6..3432b0bc4f 100644 --- a/src/fixtures/playwright.config.ts +++ b/src/fixtures/playwright.config.ts @@ -6,6 +6,15 @@ import { defineConfig, devices } from '@playwright/test' */ // require('dotenv').config(); +const PLAYWRIGHT_START_SERVER_COMMAND = + process.env.PLAYWRIGHT_START_SERVER_COMMAND || 'npm run start-for-playwright' + +const RETRIES = process.env.PLAYWRIGHT_RETRIES + ? Number(process.env.PLAYWRIGHT_RETRIES) + : process.env.CI + ? 2 + : 0 + /** * See https://playwright.dev/docs/test-configuration. */ @@ -25,7 +34,7 @@ export default defineConfig({ /* Fail the build on CI if you accidentally left test.only in the source code. */ forbidOnly: !!process.env.CI, /* Retry on CI only */ - retries: process.env.CI ? 2 : 0, + retries: RETRIES, /* Opt out of parallel tests on CI. */ workers: process.env.PLAYWRIGHT_WORKERS ? JSON.parse(process.env.PLAYWRIGHT_WORKERS) @@ -102,7 +111,7 @@ export default defineConfig({ /* Run your local dev server before starting the tests */ webServer: { - command: 'npm run start-for-playwright', + command: PLAYWRIGHT_START_SERVER_COMMAND, port: 4000, }, }) diff --git a/src/fixtures/tests/playwright-local-dev.spec.ts b/src/fixtures/tests/playwright-local-dev.spec.ts new file mode 100644 index 0000000000..84b5cc4d33 --- /dev/null +++ b/src/fixtures/tests/playwright-local-dev.spec.ts @@ -0,0 +1,34 @@ +/** + * These tests assume you have starte the local dev server as a contributor + * would. It does *not* use fixture data. It uses real English content + * as seen in `main` or in the current branch. Therefore be careful + * with what you can expect to find. Stick to known and stable content. + * + * It's always a risk that the content changes and can break tests + * that exist to test the *code*. But these tests are ultimately there to + * do what a human would do which is: Start the server, then open the + * browser, then click around, then search, etc. + * + */ + +import { test, expect } from '@playwright/test' + +test('view home page', async ({ page }) => { + await page.goto('/') + await expect(page).toHaveTitle(/GitHub Docs/) +}) + +test('click "Get started" from home page', async ({ page }) => { + await page.goto('/') + await page.getByRole('link', { name: 'Get started' }).click() + await expect(page).toHaveTitle(/Get started with GitHub/) + await expect(page).toHaveURL(/\/en\/get-started/) +}) + +test('search "git" and get results', async ({ page }) => { + await page.goto('/') + await page.getByTestId('site-search-input').click() + await page.getByTestId('site-search-input').fill('git') + await page.getByTestId('site-search-input').press('Enter') + await expect(page.getByRole('heading', { name: /\d+ Search results for "git"/ })).toBeVisible() +}) diff --git a/src/frame/components/DefaultLayout.tsx b/src/frame/components/DefaultLayout.tsx index 0d62d40441..4163e6ff53 100644 --- a/src/frame/components/DefaultLayout.tsx +++ b/src/frame/components/DefaultLayout.tsx @@ -17,8 +17,8 @@ const MINIMAL_RENDER = Boolean(JSON.parse(process.env.MINIMAL_RENDER || 'false') type Props = { children?: React.ReactNode } export const DefaultLayout = (props: Props) => { + const mainContext = useMainContext() const { - page, error, isHomepageVersion, currentPathWithoutLanguage, @@ -27,10 +27,10 @@ export const DefaultLayout = (props: Props) => { relativePath, fullUrl, status, - } = useMainContext() + } = mainContext + const page = mainContext.page! const { t } = useTranslation(['meta', 'scroll_button']) const router = useRouter() - const metaDescription = page.introPlainText ? page.introPlainText : t('default_description') const { languages } = useLanguages() // This is only true when we do search indexing which renders every page @@ -55,6 +55,8 @@ export const DefaultLayout = (props: Props) => { ) } + const metaDescription = page.introPlainText ? page.introPlainText : t('default_description') + return ( <> diff --git a/src/frame/components/context/MainContext.tsx b/src/frame/components/context/MainContext.tsx index 906329bacc..d0c3886b83 100644 --- a/src/frame/components/context/MainContext.tsx +++ b/src/frame/components/context/MainContext.tsx @@ -97,7 +97,7 @@ export type MainContextT = { href: string } currentProduct?: ProductT - currentLayoutName: string + currentLayoutName?: string isHomepageVersion: boolean data: DataT error: string @@ -120,7 +120,7 @@ export type MainContextT = { hidden: boolean noEarlyAccessBanner: boolean applicableVersions: string[] - } + } | null enterpriseServerVersions: Array @@ -172,10 +172,9 @@ export const getMainContext = async (req: any, res: any): Promise delete req.context.site.data.ui.ms } - if (!req.context.page) { - throw new Error(`No page context (${req.url})`) - } - const { documentType } = req.context.page + const { page } = req.context + + const documentType = page ? (page.documentType as string) : undefined const ui: UIStrings = {} addUINamespaces(req, ui, DEFAULT_UI_NAMESPACES) @@ -210,11 +209,24 @@ export const getMainContext = async (req: any, res: any): Promise // as a full version string if the release candidate is set. const releaseCandidateVersion = releaseCandidate ? `enterprise-server@${releaseCandidate}` : null - return { + const pageInfo = + (page && { + documentType, + type: req.context.page.type || null, + title: req.context.page.title, + fullTitle: req.context.page.fullTitle || null, + topics: req.context.page.topics || [], + introPlainText: req.context.page?.introPlainText || null, + applicableVersions: req.context.page?.permalinks.map((obj: any) => obj.pageVersion) || [], + hidden: req.context.page.hidden || false, + noEarlyAccessBanner: req.context.page.noEarlyAccessBanner || false, + }) || + null + + const props: MainContextT = { breadcrumbs: req.context.breadcrumbs || {}, communityRedirect: req.context.page?.communityRedirect || {}, currentProduct: req.context.productMap[req.context.currentProduct] || null, - currentLayoutName: req.context.currentLayoutName, isHomepageVersion: req.context.page?.documentType === 'homepage', error: req.context.error ? req.context.error.toString() : '', data: { @@ -230,18 +242,7 @@ export const getMainContext = async (req: any, res: any): Promise }, currentCategory: req.context.currentCategory || '', currentPathWithoutLanguage: req.context.currentPathWithoutLanguage, - relativePath: req.context.page?.relativePath, - page: { - documentType, - type: req.context.page.type || null, - title: req.context.page.title, - fullTitle: req.context.page.fullTitle, - topics: req.context.page.topics || [], - introPlainText: req.context.page?.introPlainText, - applicableVersions: req.context.page?.permalinks.map((obj: any) => obj.pageVersion) || [], - hidden: req.context.page.hidden || false, - noEarlyAccessBanner: req.context.page.noEarlyAccessBanner || false, - }, + page: pageInfo, enterpriseServerReleases: pick(req.context.enterpriseServerReleases, [ 'isOldestReleaseDeprecated', 'oldestSupported', @@ -267,6 +268,14 @@ export const getMainContext = async (req: any, res: any): Promise status: res.statusCode, fullUrl: req.protocol + '://' + req.get('host') + req.originalUrl, } + + if (req.context.currentLayoutName) { + props.currentLayoutName = req.context.currentLayoutName + } + if (req.context.page?.relativePath) { + props.relativePath = req.context.page.relativePath + } + return props } export const MainContext = createContext(null) diff --git a/src/frame/components/page-header/HeaderNotifications.tsx b/src/frame/components/page-header/HeaderNotifications.tsx index 2d9a2c68c1..8ca7b86c68 100644 --- a/src/frame/components/page-header/HeaderNotifications.tsx +++ b/src/frame/components/page-header/HeaderNotifications.tsx @@ -25,7 +25,9 @@ type Notif = { export const HeaderNotifications = () => { const router = useRouter() const { currentVersion } = useVersion() - const { relativePath, allVersions, data, currentPathWithoutLanguage, page } = useMainContext() + const mainContext = useMainContext() + const { relativePath, allVersions, data, currentPathWithoutLanguage } = mainContext + const page = mainContext.page! const { userLanguage, setUserLanguageCookie } = useUserLanguage() const { languages } = useLanguages() diff --git a/src/frame/middleware/next.js b/src/frame/middleware/next.js index 1178e7568b..71735d2b6f 100644 --- a/src/frame/middleware/next.js +++ b/src/frame/middleware/next.js @@ -8,24 +8,14 @@ export const nextHandleRequest = nextApp.getRequestHandler() await nextApp.prepare() function renderPageWithNext(req, res, next) { - const isNextDataRequest = req.path.startsWith('/_next') && !req.path.startsWith('/_next/data') - - if ( - isNextDataRequest && - // In local development, the very first request for a _next/static file - // triggers Nextjs to build it. So we need to let Nextjs handle that. - // But once it's built, we can handle it ourselves. - !req.path.startsWith('/_next/webpack-hmr') && - // If the file doesn't exist on disk, and fell through our express.static - // for the `_next/static` prefix, it means the file does not exist. - // And trying to handle it will trigger the run of - // getServerSideProps() in `pages/index.tsx` which assumes there exists - // a page always. - !/_next\/static\/webpack\/[a-f0-9]+\.webpack\.hot-update\.json/.test(req.path) - ) { + if (req.path.startsWith('/_next') && !req.path.startsWith('/_next/data')) { return nextHandleRequest(req, res) } + // Note that URLs like `/_next/webpack-hmr` and + // '/_next/static/webpack/64e44ef62e261d3a.webpack.hot-update.json' has to + // go through here. + return next() } diff --git a/src/graphql/pages/explorer.tsx b/src/graphql/pages/explorer.tsx index 5bb93bf0f1..449da0a8f3 100644 --- a/src/graphql/pages/explorer.tsx +++ b/src/graphql/pages/explorer.tsx @@ -9,7 +9,11 @@ type Props = { graphqlExplorerUrl: string } export default function GQLExplorer({ mainContext, graphqlExplorerUrl }: Props) { - const { page } = mainContext + // Use TypeScript's "not null assertion" because `context.page` should + // will present in main context if it's gotten to the stage of React + // rendering. + const page = mainContext.page! + const graphiqlRef = useRef(null) useEffect(() => { diff --git a/src/landings/components/ArticleList.tsx b/src/landings/components/ArticleList.tsx index 341cdba971..cbb98444d8 100644 --- a/src/landings/components/ArticleList.tsx +++ b/src/landings/components/ArticleList.tsx @@ -22,7 +22,12 @@ export const ArticleList = ({ articles, }: ArticleListPropsT) => { const { t } = useTranslation('product_landing') - const { page } = useMainContext() + const mainContext = useMainContext() + // Use TypeScript's "not null assertion" because `mainContext.page` should + // will present in mainContext if it's gotten to the stage of React + // rendering. + const page = mainContext.page! + return ( <> {title && ( diff --git a/src/landings/pages/product.tsx b/src/landings/pages/product.tsx index 0c73891baf..8c66052f7f 100644 --- a/src/landings/pages/product.tsx +++ b/src/landings/pages/product.tsx @@ -99,7 +99,12 @@ const GlobalPage = ({ ) } else { - throw new Error('No context provided to page') + // In local dev, when Next.js needs the initial compiled version + // it will request `/_next/static/webpack/$HASH.webpack.hot-update.json` + // and then we just let the `content` be undefined. + if (!router.asPath.startsWith('/_next/static/')) { + throw new Error('No context provided to page') + } } return {content} @@ -130,7 +135,7 @@ export const getServerSideProps: GetServerSideProps = async (context) => if (props.tocLandingContext.currentLearningTrack?.trackName) { additionalUINamespaces.push('learning_track_nav') } - } else { + } else if (props.mainContext.page) { // All articles that might have hover cards needs this additionalUINamespaces.push('popovers') diff --git a/src/search/components/NoQuery.tsx b/src/search/components/NoQuery.tsx index ff7301f721..753d450344 100644 --- a/src/search/components/NoQuery.tsx +++ b/src/search/components/NoQuery.tsx @@ -5,7 +5,11 @@ import { useTranslation } from 'src/languages/components/useTranslation' export function NoQuery() { const { t } = useTranslation(['search']) - const { page } = useMainContext() + const mainContext = useMainContext() + // Use TypeScript's "not null assertion" because `context.page` should + // will present in main context if it's gotten to the stage of React + // rendering. + const page = mainContext.page! return ( <> diff --git a/src/search/components/index.tsx b/src/search/components/index.tsx index f61133a3cf..cbc81a300a 100644 --- a/src/search/components/index.tsx +++ b/src/search/components/index.tsx @@ -30,7 +30,9 @@ export function Search({ search }: Props) { const { results, validationErrors } = search const hasQuery = Boolean((query && query.trim()) || '') - let pageTitle = documentPage.fullTitle + // Mostly to satisfy TypeScript because the useMainContext hook + // is run on every request and every request doesn't have a page. + let pageTitle = documentPage?.fullTitle || 'Search' if (hasQuery) { pageTitle = `${t('search_results_for')} "${query.trim()}"` if (currentVersion !== DEFAULT_VERSION) { diff --git a/src/versions/components/VersionPicker.tsx b/src/versions/components/VersionPicker.tsx index 6648c1be61..2c12c89228 100644 --- a/src/versions/components/VersionPicker.tsx +++ b/src/versions/components/VersionPicker.tsx @@ -15,7 +15,12 @@ type Props = { export const VersionPicker = ({ xs }: Props) => { const router = useRouter() const { currentVersion } = useVersion() - const { allVersions, page, enterpriseServerVersions } = useMainContext() + const mainContext = useMainContext() + // Use TypeScript's "not null assertion" because mainContext.page` should + // will present in mainContext if it's gotten to the stage of React + // rendering. + const page = mainContext.page! + const { allVersions, enterpriseServerVersions } = mainContext const { t } = useTranslation(['pages', 'picker']) if (page.applicableVersions && page.applicableVersions.length < 1) {