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

feat(develop): add explicit express handler for page-data requests #27882

Merged
merged 6 commits into from
Nov 12, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,9 @@ describe(`redirect`, () => {
pagePath: `/404.html`,
filter: `page-data`,
})
cy.task(`blockPageComponent`, {
path: `pages/404.js`,
})
})

after(() => {
Expand Down
31 changes: 26 additions & 5 deletions e2e-tests/development-runtime/cypress/plugins/block-resources.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ const fs = require(`fs-extra`)
const path = require(`path`)
const glob = require(`glob`)

const publicDir = path.join(__dirname, `..`, `..`, `public`)
const siteDir = path.join(__dirname, `..`, `..`)
const srcDir = path.join(siteDir, `src`)
const publicDir = path.join(siteDir, `public`)

const moveAsset = (from, to) => {
const fromExists = fs.existsSync(from)
Expand All @@ -18,7 +20,7 @@ const moveAsset = (from, to) => {
}
}

const restorePageData = hiddenPath => {
const restoreAsset = hiddenPath => {
if (path.basename(hiddenPath).charAt(0) !== `_`) {
throw new Error(`hiddenPath should have _ prefix`)
}
Expand Down Expand Up @@ -51,10 +53,28 @@ const blockAssetsForPage = ({ pagePath, filter }) => {
return null
}

function blockPageComponent({ path: pageComponentPath }) {
const hiddenPath = path.join(
path.dirname(pageComponentPath),
`_` + path.basename(pageComponentPath)
)

moveAsset(path.join(srcDir, pageComponentPath), path.join(srcDir, hiddenPath))
return null
}

const restore = () => {
const globPattern = path.join(publicDir, `/page-data/**`, `_page-data.json`)
const hiddenPageDatas = glob.sync(globPattern)
hiddenPageDatas.forEach(restorePageData)
const hiddenPageDataGlobPattern = path.join(
publicDir,
`/page-data/**`,
`_page-data.json`
)
const hiddenPageDatas = glob.sync(hiddenPageDataGlobPattern)
hiddenPageDatas.forEach(restoreAsset)

const hiddenPageComponentGlobPattern = path.join(srcDir, `**`, `_*`)
vladar marked this conversation as resolved.
Show resolved Hide resolved
const hiddenPageComponents = glob.sync(hiddenPageComponentGlobPattern)
hiddenPageComponents.forEach(restoreAsset)

console.log(`Restored resources`)
return null
Expand All @@ -63,4 +83,5 @@ const restore = () => {
module.exports = {
restoreAllBlockedResources: restore,
blockAssetsForPage,
blockPageComponent,
}
23 changes: 19 additions & 4 deletions packages/gatsby/src/internal-plugins/prod-404/gatsby-node.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,28 @@
let created404 = false
const { emitter } = require(`../../redux`)
const { boundActionCreators } = require(`../../redux/actions`)

const PROD_404_PAGE_PATH = `/404.html`

let page404 = null
exports.onCreatePage = ({ page, store, actions }) => {
// Copy /404/ to /404.html as many static site hosts expect
// site 404 pages to be named this.
// https://www.gatsbyjs.org/docs/add-404-page/
if (!created404 && /^\/?404\/?$/.test(page.path)) {
if (!page404 && /^\/?404\/?$/.test(page.path)) {
actions.createPage({
...page,
path: `/404.html`,
path: PROD_404_PAGE_PATH,
})
created404 = true
page404 = page
}
}

emitter.on(`DELETE_PAGE`, action => {
if (page404 && action.payload.path === page404.path) {
boundActionCreators.deletePage({
...page404,
path: PROD_404_PAGE_PATH,
})
vladar marked this conversation as resolved.
Show resolved Hide resolved
page404 = null
}
})
Comment on lines +20 to +28
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • prod-404 internal plugin is actually also used in gatsby develop
  • removing /404/ page (that this plugin clones into /404.html) wasn't removing clone which meant that 404 page removals didn't actually removed /404.html page

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know much about this plugin, just want to understand: why does it hook into onCreatePage instead of creating its own page? And where do we actually create the source 404 page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so explanation for this plugin - when user creates src/pages/404.js, the actual path for page is /404/ (that's what gatsby-plugin-page-creator does) which translated to /404/index.html when generating .html file - this is fine, but a lot of hosting solutions handle /404.html as potential default to show 404 page (I think Netlify does this for sure). So we do special case 404 kind of page and create "duplicate" page for it

The src/pages/404.js is also not only case when we do this - if user creates page programatically in createPages - this will work too.

Note that this is mostly needed for production builds, but our shared runtime (both dev and prod) also uses /404.html in multiple places, so the way this plugin works impact both develop and prod.

4 changes: 4 additions & 0 deletions packages/gatsby/src/utils/page-data.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ export function fixedPagePath(pagePath: string): string {
return pagePath === `/` ? `index` : pagePath
}

export function reverseFixedPagePath(pageDataRequestPath: string): string {
return pageDataRequestPath === `index` ? `/` : pageDataRequestPath
}

function getFilePath(publicDir: string, pagePath: string): string {
return path.join(
publicDir,
Expand Down
37 changes: 37 additions & 0 deletions packages/gatsby/src/utils/start-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,12 @@ import https from "https"
import { developStatic } from "../commands/develop-static"
import withResolverContext from "../schema/context"
import { websocketManager, WebsocketManager } from "../utils/websocket-manager"
import { reverseFixedPagePath, readPageData } from "./page-data"
import { findPageByPath } from "./find-page-by-path"
import { slash } from "gatsby-core-utils"
import apiRunnerNode from "../utils/api-runner-node"
import { Express } from "express"
import * as path from "path"

import { Stage, IProgram } from "../commands/types"
import JestWorker from "jest-worker"
Expand Down Expand Up @@ -213,6 +216,40 @@ export async function startServer(
res.end()
})

app.get(
`/page-data/:pagePath(*)/page-data.json`,
async (req, res, next): Promise<void> => {
const requestedPagePath = req.params.pagePath
if (!requestedPagePath) {
next()
return
}

const potentialPagePath = reverseFixedPagePath(requestedPagePath)
const page = findPageByPath(store.getState(), potentialPagePath, false)

if (page) {
try {
const pageData = await readPageData(
path.join(store.getState().program.directory, `public`),
page.path
)
Comment on lines +233 to +236
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to clarify - we will actually use the new page data utility here? (i.e. #27939)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

res.status(200).send(pageData)
return
} catch (e) {
report.error(
`Error loading a result for the page query in "${requestedPagePath}" / "${potentialPagePath}". Query was not run and no cached result was found.`,
e
)
}
}

res.status(404).send({
path: potentialPagePath,
})
}
)

// Disable directory indexing i.e. serving index.html from a directory.
// This can lead to serving stale html files during development.
//
Expand Down