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

Automatic file validation for components in src/pages #3881

Merged
merged 8 commits into from
Feb 9, 2018
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
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
"eslint-plugin-react": "^7.3.0",
"flow-bin": "^0.42.0",
"glob": "^7.1.1",
"jest": "^20.0.4",
"jest-cli": "^20.0.4",
"jest": "^22.1.4",
"jest-cli": "^22.1.4",
"lerna": "^2.1.1",
"npm-run-all": "4.1.2",
"plop": "^1.8.1",
Expand Down
17 changes: 17 additions & 0 deletions packages/gatsby/__mocks__/fs.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict'

const fs = jest.genMockFromModule(`fs`)

let mockFiles = {}
function __setMockFiles(newMockFiles) {
mockFiles = Object.assign({}, newMockFiles)
}

function readFileSync(filePath, parser) {
return mockFiles[filePath]
}

fs.__setMockFiles = __setMockFiles
fs.readFileSync = readFileSync

module.exports = fs
30 changes: 27 additions & 3 deletions packages/gatsby/src/redux/__tests__/pages.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,29 @@
'use strict'

const glob = require(`glob`)
const reducer = require(`../reducers/pages`)
const { actions } = require(`../actions`)

jest.mock(`fs`)

Date.now = jest.fn(
() =>
// const diff = new Date().getTime() - start
1482363367071 // + diff
)

glob.sync = jest.fn(() => ``)

describe(`Add pages`, () => {
it(`allows you to add pages`, () => {
const MOCK_FILE_INFO = {
'/whatever/index.js': `import React from 'react'; export default Page;`,
'/whatever2/index.js': `import React from 'react'; export default Page;`,
}
beforeEach(() => {
// Set up some mocked out file info before each test
require(`fs`).__setMockFiles(MOCK_FILE_INFO)
})
test(`allows you to add pages`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand All @@ -22,16 +37,18 @@ describe(`Add pages`, () => {
})

it(`Fails if path is missing`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
component: `/whatever/index.js`,
component: `/path/to/file1.js`,
},
{ id: `test`, name: `test` }
)
expect(action).toMatchSnapshot()
})

it(`Fails if component path is missing`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/whatever/`,
Expand All @@ -42,6 +59,7 @@ describe(`Add pages`, () => {
})

it(`Fails if the component path isn't absolute`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/whatever/`,
Expand All @@ -53,6 +71,7 @@ describe(`Add pages`, () => {
})

it(`adds an initial forward slash if the user doesn't`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `hi/`,
Expand All @@ -65,6 +84,7 @@ describe(`Add pages`, () => {
})

it(`allows you to add pages with context`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand All @@ -81,6 +101,7 @@ describe(`Add pages`, () => {
})

it(`allows you to add pages with matchPath`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand All @@ -95,6 +116,7 @@ describe(`Add pages`, () => {
})

it(`allows you to add multiple pages`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand All @@ -116,6 +138,7 @@ describe(`Add pages`, () => {
})

it(`allows you to update existing pages (based on path)`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand All @@ -140,6 +163,7 @@ describe(`Add pages`, () => {
})

it(`allows you to delete paths`, () => {
const { actions } = require(`../actions`)
const action = actions.createPage(
{
path: `/hi/`,
Expand Down
68 changes: 67 additions & 1 deletion packages/gatsby/src/redux/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const { bindActionCreators } = require(`redux`)
const { stripIndent } = require(`common-tags`)
const glob = require(`glob`)
const path = require(`path`)

const fs = require(`fs`)
const { joinPath } = require(`../utils/path`)
const {
getNode,
Expand Down Expand Up @@ -187,6 +187,72 @@ actions.createPage = (page: PageInput, plugin?: Plugin, traceId?: string) => {
return null
}

// Validate that the page component imports React and exports something
// (hopefully a component).
if (!internalPage.component.includes(`/.cache/`)) {
const fileContent = fs.readFileSync(internalPage.component, `utf-8`)
let notEmpty = true
let includesReactImport = true
let includesDefaultExport = true

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

if (!fileContent.includes(`React`)) {
includesReactImport = false
}
if (
!fileContent.includes(`export default`) &&
!fileContent.includes(`module.exports`)
) {
includesDefaultExport = false
}
if (!notEmpty || !includesDefaultExport || !includesReactImport) {
const relativePath = path.relative(
store.getState().program.directory,
internalPage.component
)

if (!notEmpty) {
console.log(``)
console.log(
`You have an empty file in the "src/pages" directory at "${relativePath}". Please remove it or make it a valid component`
)
console.log(``)
process.exit(1)
}

console.log(``)
console.log(``)
console.log(
`The page component at "${relativePath}" didn't pass validation`
)

if (!includesReactImport) {
console.log(``)
console.log(
`You must import React at the top of the file for a React component to be valid`
)
console.log(``)
console.log(`Add the following to the top of the component:`)
console.log(``)
console.log(` import React from 'react'`)
console.log(``)
}

if (!includesDefaultExport) {
console.log(``)
console.log(
`The page component must export a React component for it to be valid`
Copy link
Contributor

Choose a reason for hiding this comment

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

These warnings are giving false positives on my site where the pages are .md files that are turned into components by webpack. I think this warning is a bit zealous, at the very least it probably should ignore none js files

Copy link

Choose a reason for hiding this comment

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

likewise, it fails to recognize

export {
 SomeRandomReactCompoment as default
}

would a PR adding "as default" to the above checks be acceptable?

Copy link
Contributor

Choose a reason for hiding this comment

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

@riwsky sure - that seems reasonable!

)
console.log(``)
}

process.exit(1)
}
}

return {
type: `CREATE_PAGE`,
plugin,
Expand Down
Loading