Skip to content

Commit

Permalink
chore(gatsby): Optimise create page action validation (#24684)
Browse files Browse the repository at this point in the history
* chore(gatsby): Optimise create page action validation

* Remove change that will go iin a different PR

* Correctly use passed plugin name

* Fix snapshot

* Switch cache to use Set

Co-authored-by: Ward Peeters <[email protected]>

Co-authored-by: Ward Peeters <[email protected]>
Co-authored-by: gatsbybot <[email protected]>
  • Loading branch information
3 people authored Jun 4, 2020
1 parent 317aa65 commit 64858b1
Show file tree
Hide file tree
Showing 3 changed files with 143 additions and 113 deletions.
18 changes: 9 additions & 9 deletions packages/gatsby/src/redux/__tests__/__snapshots__/pages.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ Map {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -64,7 +64,7 @@ Map {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHiPizza",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/pizza/",
"pluginCreatorId": "test",
Expand All @@ -82,7 +82,7 @@ Object {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -104,7 +104,7 @@ Map {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -124,7 +124,7 @@ Object {
"id": 123,
},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -148,7 +148,7 @@ Map {
"id": 123,
},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -166,7 +166,7 @@ Object {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": "/hi-from-somewhere-else/",
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -188,7 +188,7 @@ Map {
"componentChunkName": "component---whatever-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": "/hi-from-somewhere-else/",
"path": "/hi/",
"pluginCreatorId": "test",
Expand All @@ -207,7 +207,7 @@ Map {
"componentChunkName": "component---whatever-2-index-js",
"context": Object {},
"internalComponentName": "ComponentHi",
"isCreatedByStatefulCreatePages": undefined,
"isCreatedByStatefulCreatePages": false,
"matchPath": undefined,
"path": "/hi/",
"pluginCreatorId": "test",
Expand Down
133 changes: 29 additions & 104 deletions packages/gatsby/src/redux/actions/public.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,13 @@ const { stripIndent } = require(`common-tags`)
const report = require(`gatsby-cli/lib/reporter`)
const { platform } = require(`os`)
const path = require(`path`)
const fs = require(`fs`)
const { trueCasePathSync } = require(`true-case-path`)
const url = require(`url`)
const { slash } = require(`gatsby-core-utils`)
const { hasNodeChanged, getNode } = require(`../../db/nodes`)
const sanitizeNode = require(`../../db/sanitize-node`)
const { store } = require(`..`)
const fileExistsSync = require(`fs-exists-cached`).sync
const { validatePageComponent } = require(`../../utils/validate-page-component`)
import { nodeSchema } from "../../joi-schemas/joi"
const { generateComponentChunkName } = require(`../../utils/js-chunk-names`)
const {
Expand Down Expand Up @@ -135,8 +134,14 @@ const hasWarnedForPageComponentInvalidContext = new Set()
const hasWarnedForPageComponentInvalidCasing = new Set()
const hasErroredBecauseOfNodeValidation = new Set()
const pageComponentCache = {}
const fileOkCache = {}

const reservedFields = [
`path`,
`matchPath`,
`component`,
`componentChunkName`,
`pluginCreator___NODE`,
`pluginCreatorId`,
]
/**
* Create a page. See [the guide on creating and modifying pages](/docs/creating-and-modifying-pages/)
* for detailed documentation about creating pages.
Expand Down Expand Up @@ -187,22 +192,14 @@ actions.createPage = (

// Validate that the context object doesn't overlap with any core page fields
// as this will cause trouble when running graphql queries.
if (_.isObject(page.context)) {
const reservedFields = [
`path`,
`matchPath`,
`component`,
`componentChunkName`,
`pluginCreator___NODE`,
`pluginCreatorId`,
]
const invalidFields = Object.keys(_.pick(page.context, reservedFields))

const singularMessage = `${name} used a reserved field name in the context object when creating a page:`
const pluralMessage = `${name} used reserved field names in the context object when creating a page:`
if (typeof page.context === `object`) {
const invalidFields = reservedFields.filter(field => field in page.context)

if (invalidFields.length > 0) {
const error = `${
invalidFields.length === 1 ? singularMessage : pluralMessage
invalidFields.length === 1
? `${name} used a reserved field name in the context object when creating a page:`
: `${name} used reserved field names in the context object when creating a page:`
}
${invalidFields.map(f => ` * "${f}"`).join(`\n`)}
Expand Down Expand Up @@ -268,35 +265,21 @@ ${reservedFields.map(f => ` * "${f}"`).join(`\n`)}
page.component = pageComponentPath
}

// Don't check if the component exists during tests as we use a lot of fake
// component paths.
if (process.env.NODE_ENV !== `test`) {
if (!fileExistsSync(page.component)) {
report.panic({
id: `11325`,
context: {
pluginName: name,
pageObject: page,
component: page.component,
},
})
}
}
if (!path.isAbsolute(page.component)) {
// Don't log out when testing
const { error, message, panicOnBuild } = validatePageComponent(
page,
store.getState().program.directory,
name
)

if (error) {
if (process.env.NODE_ENV !== `test`) {
report.panic({
id: `11326`,
context: {
pluginName: name,
pageObject: page,
component: page.component,
},
})
} else {
const message = `${name} must set the absolute path to the page component when create creating a page`
return message
if (panicOnBuild) {
report.panicOnBuild(error)
} else {
report.panic(error)
}
}
return message
}

// check if we've processed this component path
Expand Down Expand Up @@ -402,8 +385,7 @@ ${reservedFields.map(f => ` * "${f}"`).join(`\n`)}
component: page.component,
componentChunkName: generateComponentChunkName(page.component),
isCreatedByStatefulCreatePages:
actionOptions &&
actionOptions.traceId === `initial-createPagesStatefully`,
actionOptions?.traceId === `initial-createPagesStatefully`,
// Ensure the page has a context object
context: page.context || {},
updatedAt: Date.now(),
Expand All @@ -414,63 +396,6 @@ ${reservedFields.map(f => ` * "${f}"`).join(`\n`)}
internalPage.path = `/${internalPage.path}`
}

// Validate that the page component imports React and exports something
// (hopefully a component).
//
// Only run validation once during builds.
if (
!internalPage.component.includes(`/.cache/`) &&
process.env.NODE_ENV === `production` &&
!fileOkCache[internalPage.component]
) {
const fileName = internalPage.component
const fileContent = fs.readFileSync(fileName, `utf-8`)
let notEmpty = true
let includesDefaultExport = true

if (fileContent === ``) {
notEmpty = false
}

if (
!fileContent.includes(`export default`) &&
!fileContent.includes(`module.exports`) &&
!fileContent.includes(`exports.default`) &&
!fileContent.includes(`exports["default"]`) &&
!fileContent.match(/export \{.* as default.*\}/s) &&
// this check only applies to js and ts, not mdx
/\.(jsx?|tsx?)/.test(path.extname(fileName))
) {
includesDefaultExport = false
}
if (!notEmpty || !includesDefaultExport) {
const relativePath = path.relative(
store.getState().program.directory,
fileName
)

if (!notEmpty) {
report.panicOnBuild({
id: `11327`,
context: {
relativePath,
},
})
}

if (!includesDefaultExport) {
report.panicOnBuild({
id: `11328`,
context: {
fileName,
},
})
}
}

fileOkCache[internalPage.component] = true
}

const oldPage: Page = store.getState().pages.get(internalPage.path)
const contextModified =
!!oldPage && !_.isEqual(oldPage.context, internalPage.context)
Expand Down
105 changes: 105 additions & 0 deletions packages/gatsby/src/utils/validate-page-component.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
import path from "path"
import fs from "fs-extra"
import { IGatsbyPage } from "../redux/types"

const validationCache = new Set<string>()

interface IErrorMeta {
id: string
context: Record<string, unknown>
}

export function validatePageComponent(
page: IGatsbyPage,
directory: string,
pluginName: string
): { message?: string; error?: IErrorMeta; panicOnBuild?: boolean } {
const { component } = page
if (!component) {
throw new Error(`11322`)
}
if (validationCache.has(component)) {
return {}
}
if (!path.isAbsolute(component)) {
return {
error: {
id: `11326`,
context: {
pluginName,
pageObject: page,
component: component,
},
},
message: `${pluginName} must set the absolute path to the page component when create creating a page`,
}
}

if (process.env.NODE_ENV !== `test`) {
if (!fs.existsSync(component)) {
return {
error: {
id: `11325`,
context: {
pluginName,
pageObject: page,
component: component,
},
},
}
}
}

// Validate that the page component imports React and exports something
// (hopefully a component).
//

if (
!component.includes(`/.cache/`) &&
process.env.NODE_ENV === `production`
) {
const fileContent = fs.readFileSync(component, `utf-8`)

if (fileContent === ``) {
const relativePath = path.relative(directory, component)

return {
error: {
id: `11327`,
context: {
relativePath,
},
},
panicOnBuild: true,
}
}

const includesDefaultExport =
fileContent.includes(`export default`) ||
fileContent.includes(`module.exports`) ||
fileContent.includes(`exports.default`) ||
fileContent.includes(`exports["default"]`) ||
fileContent.match(/export \{.* as default.*\}/s) ||
// this check only applies to js and ts, not mdx
/\.(jsx?|tsx?)/.test(path.extname(component))

if (!includesDefaultExport) {
return {
error: {
id: `11328`,
context: {
component,
},
},
panicOnBuild: true,
}
}
}

validationCache.add(component)
return {}
}

export function clearValidationCache(): void {
validationCache.clear()
}

0 comments on commit 64858b1

Please sign in to comment.