From 64858b12b34a3fab45b1bb06a6b65803231569e2 Mon Sep 17 00:00:00 2001 From: Matt Kane Date: Thu, 4 Jun 2020 09:11:05 +0100 Subject: [PATCH] chore(gatsby): Optimise create page action validation (#24684) * 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 Co-authored-by: Ward Peeters Co-authored-by: gatsbybot --- .../__tests__/__snapshots__/pages.ts.snap | 18 +-- packages/gatsby/src/redux/actions/public.js | 133 ++++-------------- .../src/utils/validate-page-component.ts | 105 ++++++++++++++ 3 files changed, 143 insertions(+), 113 deletions(-) create mode 100644 packages/gatsby/src/utils/validate-page-component.ts diff --git a/packages/gatsby/src/redux/__tests__/__snapshots__/pages.ts.snap b/packages/gatsby/src/redux/__tests__/__snapshots__/pages.ts.snap index 570a607e499d1..4db23a003c44b 100644 --- a/packages/gatsby/src/redux/__tests__/__snapshots__/pages.ts.snap +++ b/packages/gatsby/src/redux/__tests__/__snapshots__/pages.ts.snap @@ -52,7 +52,7 @@ Map { "componentChunkName": "component---whatever-index-js", "context": Object {}, "internalComponentName": "ComponentHi", - "isCreatedByStatefulCreatePages": undefined, + "isCreatedByStatefulCreatePages": false, "matchPath": undefined, "path": "/hi/", "pluginCreatorId": "test", @@ -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", @@ -82,7 +82,7 @@ Object { "componentChunkName": "component---whatever-index-js", "context": Object {}, "internalComponentName": "ComponentHi", - "isCreatedByStatefulCreatePages": undefined, + "isCreatedByStatefulCreatePages": false, "matchPath": undefined, "path": "/hi/", "pluginCreatorId": "test", @@ -104,7 +104,7 @@ Map { "componentChunkName": "component---whatever-index-js", "context": Object {}, "internalComponentName": "ComponentHi", - "isCreatedByStatefulCreatePages": undefined, + "isCreatedByStatefulCreatePages": false, "matchPath": undefined, "path": "/hi/", "pluginCreatorId": "test", @@ -124,7 +124,7 @@ Object { "id": 123, }, "internalComponentName": "ComponentHi", - "isCreatedByStatefulCreatePages": undefined, + "isCreatedByStatefulCreatePages": false, "matchPath": undefined, "path": "/hi/", "pluginCreatorId": "test", @@ -148,7 +148,7 @@ Map { "id": 123, }, "internalComponentName": "ComponentHi", - "isCreatedByStatefulCreatePages": undefined, + "isCreatedByStatefulCreatePages": false, "matchPath": undefined, "path": "/hi/", "pluginCreatorId": "test", @@ -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", @@ -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", @@ -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", diff --git a/packages/gatsby/src/redux/actions/public.js b/packages/gatsby/src/redux/actions/public.js index 5accdb9e50e30..dd3cdd63e43eb 100644 --- a/packages/gatsby/src/redux/actions/public.js +++ b/packages/gatsby/src/redux/actions/public.js @@ -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 { @@ -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. @@ -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`)} @@ -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 @@ -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(), @@ -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) diff --git a/packages/gatsby/src/utils/validate-page-component.ts b/packages/gatsby/src/utils/validate-page-component.ts new file mode 100644 index 0000000000000..ca7ee4b524537 --- /dev/null +++ b/packages/gatsby/src/utils/validate-page-component.ts @@ -0,0 +1,105 @@ +import path from "path" +import fs from "fs-extra" +import { IGatsbyPage } from "../redux/types" + +const validationCache = new Set() + +interface IErrorMeta { + id: string + context: Record +} + +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() +}