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(gatsby): enable richer error handling for unknown APIs #16105

Merged
merged 13 commits into from
Aug 11, 2019
4 changes: 1 addition & 3 deletions packages/gatsby-cli/src/structured-errors/construct-error.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,7 @@ const constructError = ({ details }) => {
...result,
text: result.text(details.context),
stack: details.error ? stackTrace.parse(details.error) : [],
docsUrl: result.docsUrl
? result.docsUrl
: `https://gatsby.dev/issue-how-to`,
docsUrl: result.docsUrl || `https://gatsby.dev/issue-how-to`,
}

// validate
Expand Down
30 changes: 30 additions & 0 deletions packages/gatsby-cli/src/structured-errors/error-map.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
const { stripIndent } = require(`common-tags`)

const errorMap = {
"": {
text: context => {
Expand Down Expand Up @@ -160,6 +162,34 @@ const errorMap = {
}`,
level: `ERROR`,
},
// invalid or deprecated APIs
"11329": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a pattern to these errors? Were the error codes chosen for a reason, or am I correct in presuming it's effectively an incrementing id that just makes things easier to Google?

Also next-level would be to generate docs pages based on these structured errors, and then provide even richer detail e.g.

To learn more about fixing this error, please visit https://gatsby.dev/11329

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no special meaning to codes. Actually if codes have less patterns the better ;) We want them to be easily google-able (for example to find related issues if linked docs are not enough). So best avoid codes like 11111, 12345 that have higher chance of returning unrelated search results.

One thing that is not really specified, but I think could make sense here, is to not increment by 1 when adding new structured error (if new error is related to different thing than last one).
As example you can check error related to loading gatsby-config - those occupy 10123-10125 range right now and there are slots after that to add more related errors in the future and still keep those grouped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should just print a prefix maybe then? GTSB-12345 is pretty unique!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pieh circling back to this. Few questions:

One thing that is not really specified, but I think could make sense here, is to not increment by 1 when adding new structured error

What are you proposing here exactly?

those occupy 10123-10125 range right now and there are slots after that to add more related errors in the future and still keep those grouped.

I'm happy to add it there (what you may be saying?) but those are config errors? This could be a gatsby-node.js error, but it wouldn't be a config error necessarily.

text: context =>
[
stripIndent(`
Your plugins must export known APIs from their gatsby-${
context.exportType
}.js.

See https://www.gatsbyjs.org/docs/${
context.exportType
}-apis/ for the list of Gatsby ${context.exportType} APIs.
`),
]
.concat([``].concat(context.errors))
.concat(
context.fixes.length > 0
? [
``,
`Some of the following may help fix the error(s):`,
``,
...context.fixes.map(fix => `- ${fix}`),
]
: []
)
.join(`\n`),
level: `ERROR`,
},
}

module.exports = { errorMap, defaultError: errorMap[``] }
5 changes: 4 additions & 1 deletion packages/gatsby/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -30,5 +30,8 @@ decls
dist

# built files
cache-dir/commonjs/
apis.json
cache-dir/commonjs/

# cached files
/latest-apis.json
3 changes: 3 additions & 0 deletions packages/gatsby/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
"@reach/router": "^1.1.1",
"address": "1.0.3",
"autoprefixer": "^9.6.0",
"axios": "^0.19.0",
"babel-core": "7.0.0-bridge.0",
"babel-eslint": "^9.0.0",
"babel-loader": "^8.0.0",
Expand Down Expand Up @@ -157,6 +158,7 @@
"dist",
"graphql.js",
"index.d.ts",
"scripts/postinstall.js",
"utils.js"
],
"homepage": "https://github.com/gatsbyjs/gatsby/tree/master/packages/gatsby#readme",
Expand Down Expand Up @@ -192,6 +194,7 @@
"build:src": "babel src --out-dir dist --source-maps --verbose --ignore **/gatsby-cli.js,src/internal-plugins/dev-404-page/raw_dev-404-page.js,**/__tests__",
"clean-test-bundles": "find test/ -type f -name bundle.js* -exec rm -rf {} +",
"prebuild": "rimraf dist && rimraf cache-dir/commonjs",
"postinstall": "node scripts/postinstall.js",
"prepare": "cross-env NODE_ENV=production npm run build",
"watch": "rimraf dist && mkdir dist && npm run build:internal-plugins && npm run build:rawfiles && npm run build:src -- --watch"
},
Expand Down
6 changes: 6 additions & 0 deletions packages/gatsby/scripts/postinstall.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
try {
const getLatestAPIs = require('../dist/utils/get-latest-apis')
getLatestAPIs()
} catch (e) {
// we're probably just bootstrapping and not published yet!
}
5 changes: 4 additions & 1 deletion packages/gatsby/src/bootstrap/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const telemetry = require(`gatsby-telemetry`)

const apiRunnerNode = require(`../utils/api-runner-node`)
const getBrowserslist = require(`../utils/browserslist`)
const getLatestAPIs = require(`../utils/get-latest-apis`)
const { store, emitter } = require(`../redux`)
const loadPlugins = require(`./load-plugins`)
const loadThemes = require(`./load-themes`)
Expand Down Expand Up @@ -103,9 +104,11 @@ module.exports = async (args: BootstrapArgs) => {

activity.end()

const apis = await getLatestAPIs()

activity = report.activityTimer(`load plugins`)
activity.start()
const flattenedPlugins = await loadPlugins(config, program.directory)
const flattenedPlugins = await loadPlugins(config, program.directory, apis)
activity.end()

telemetry.decorateEvent(`BUILD_END`, {
Expand Down
157 changes: 149 additions & 8 deletions packages/gatsby/src/bootstrap/load-plugins/__tests__/validate.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
jest.mock(`gatsby-cli/lib/reporter`, () => {
return {
panicOnBuild: jest.fn(),
error: jest.fn(),
warn: jest.fn(),
}
})
Expand All @@ -14,6 +15,10 @@ const {
warnOnIncompatiblePeerDependency,
} = require(`../validate`)

beforeEach(() => {
Object.keys(reporter).forEach(key => reporter[key].mockReset())
})

describe(`collatePluginAPIs`, () => {
const MOCK_RESULTS = {
"/foo/gatsby-node": [`node-1`, `node-2`],
Expand Down Expand Up @@ -55,7 +60,7 @@ describe(`collatePluginAPIs`, () => {
},
]

let result = collatePluginAPIs({ apis, flattenedPlugins })
let result = collatePluginAPIs({ currentAPIs: apis, flattenedPlugins })
expect(result).toMatchSnapshot()
})

Expand All @@ -82,15 +87,15 @@ describe(`collatePluginAPIs`, () => {
},
]

let result = collatePluginAPIs({ apis, flattenedPlugins })
let result = collatePluginAPIs({ currentAPIs: apis, flattenedPlugins })
expect(result).toMatchSnapshot()
})
})

describe(`handleBadExports`, () => {
it(`Does nothing when there are no bad exports`, async () => {
handleBadExports({
apis: {
currentAPIs: {
node: [`these`, `can`, `be`],
browser: [`anything`, `as there`],
ssr: [`are no`, `bad errors`],
Expand All @@ -103,26 +108,162 @@ describe(`handleBadExports`, () => {
})
})

it(`Calls reporter.panicOnBuild when bad exports are detected`, async () => {
it(`Calls structured error with reporter.error when bad exports are detected`, async () => {
const exportName = `foo`
handleBadExports({
apis: {
currentAPIs: {
node: [``],
browser: [``],
ssr: [`notFoo`, `bar`],
ssr: [``],
},
latestAPIs: {
node: {},
browser: {},
ssr: {},
},
badExports: {
node: [],
browser: [],
ssr: [
{
exportName: `foo`,
exportName,
pluginName: `default-site-plugin`,
},
],
},
})

expect(reporter.panicOnBuild.mock.calls.length).toBe(1)
expect(reporter.error).toHaveBeenCalledTimes(1)
expect(reporter.error).toHaveBeenCalledWith(
expect.objectContaining({
id: `11329`,
context: expect.objectContaining({
exportType: `ssr`,
errors: [
expect.stringContaining(`"${exportName}" which is not a known API`),
],
}),
})
)
})

it(`adds info on plugin if a plugin API error`, () => {
const exportName = `foo`
const pluginName = `gatsby-source-contentful`
const pluginVersion = `2.1.0`
handleBadExports({
currentAPIs: {
node: [``],
browser: [``],
ssr: [``],
},
latestAPIs: {
node: {},
browser: {},
ssr: {},
},
badExports: {
node: [],
browser: [],
ssr: [
{
exportName,
pluginName,
pluginVersion,
},
],
},
})

expect(reporter.error).toHaveBeenCalledWith(
expect.objectContaining({
context: expect.objectContaining({
errors: [
expect.stringContaining(
`${pluginName}@${pluginVersion} is using the API "${exportName}"`
),
],
}),
})
)
})

it(`Adds fixes to context if newer API introduced in Gatsby`, () => {
const version = `2.2.0`

handleBadExports({
currentAPIs: {
node: [``],
browser: [``],
ssr: [``],
},
latestAPIs: {
browser: {},
ssr: {},
node: {
validatePluginOptions: {
version,
},
},
},
badExports: {
browser: [],
ssr: [],
node: [
{
exportName: `validatePluginOptions`,
pluginName: `gatsby-source-contentful`,
},
],
},
})

expect(reporter.error).toHaveBeenCalledTimes(1)
expect(reporter.error).toHaveBeenCalledWith(
expect.objectContaining({
context: expect.objectContaining({
fixes: [`npm install gatsby@^${version}`],
}),
})
)
})

it(`adds fixes if close match/typo`, () => {
;[
[`modifyWebpackConfig`, `onCreateWebpackConfig`],
[`createPagesss`, `createPages`],
].forEach(([typoOrOldAPI, newAPI]) => {
handleBadExports({
currentAPIs: {
node: [newAPI],
browser: [``],
ssr: [``],
},
latestAPIs: {
browser: {},
ssr: {},
node: {},
},
badExports: {
browser: [],
ssr: [],
node: [
{
exportName: typoOrOldAPI,
pluginName: `default-site-plugin`,
},
],
},
})

expect(reporter.error).toHaveBeenCalledWith(
expect.objectContaining({
context: expect.objectContaining({
fixes: [`Rename "${typoOrOldAPI}" -> "${newAPI}"`],
}),
})
)
})
})
})

Expand Down
24 changes: 16 additions & 8 deletions packages/gatsby/src/bootstrap/load-plugins/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,11 @@ const {
handleMultipleReplaceRenderers,
} = require(`./validate`)

const apis = {
node: _.keys(nodeAPIs),
browser: _.keys(browserAPIs),
ssr: _.keys(ssrAPIs),
}
const getAPI = api =>
_.keys(api).reduce((merged, key) => {
merged[key] = _.keys(api[key])
return merged
}, {})

// Create a "flattened" array of plugins with all subplugins
// brought to the top-level. This simplifies running gatsby-* files
Expand All @@ -37,7 +37,15 @@ const flattenPlugins = plugins => {
return flattened
}

module.exports = async (config = {}, rootDir = null) => {
module.exports = async (config = {}, rootDir = null, latestAPIs = {}) => {
const apis = {
currentAPIs: getAPI({
browser: browserAPIs,
node: nodeAPIs,
ssr: ssrAPIs,
}),
latestAPIs,
}
// Collate internal plugins, site config plugins, site default plugins
const plugins = loadPlugins(config, rootDir)

Expand All @@ -46,12 +54,12 @@ module.exports = async (config = {}, rootDir = null) => {

// Work out which plugins use which APIs, including those which are not
// valid Gatsby APIs, aka 'badExports'
const x = collatePluginAPIs({ apis, flattenedPlugins })
const x = collatePluginAPIs({ ...apis, flattenedPlugins })
flattenedPlugins = x.flattenedPlugins
const badExports = x.badExports

// Show errors for any non-Gatsby APIs exported from plugins
handleBadExports({ apis, badExports })
handleBadExports({ ...apis, badExports })

// Show errors when ReplaceRenderer has been implemented multiple times
flattenedPlugins = handleMultipleReplaceRenderers({
Expand Down
Loading