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.

Sign up for GitHub

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

Jump to bottom

feat(gatsby): enable richer error handling for unknown APIs #16105

Merged
merged 13 commits into from
Aug 11, 2019
Merged

feat(gatsby): enable richer error handling for unknown APIs #16105

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
025874f
feat(gatsby): cache latest APIs on Gatsby install
DSchau Jul 25, 2019
c6864a7
chore: slightly refactor with axios
DSchau Jul 25, 2019
f1ea99e
feat(gatsby): introduced richer error logging for unknown APIs
DSchau Jul 26, 2019
b7a1930
chore: warning -> error
DSchau Jul 26, 2019
7861d1f
chore: remove comment I used to test
DSchau Jul 26, 2019
74b4564
fix: wrap postinstall in try/catch
DSchau Jul 26, 2019
a2dabd0
chore: slight tweaks to verbiage
DSchau Jul 26, 2019
b4665f9
fix: always prefer network
DSchau Jul 29, 2019
4c8a7d1
chore: reset changes
DSchau Jul 29, 2019
3cbd0f0
test: fix unit tests
DSchau Jul 29, 2019
bf55206
test: add a few more tests
DSchau Jul 30, 2019
d4aa82d
test: add some more tests
DSchau Jul 30, 2019
59bf7a3
test: fix windows for being a bozo
DSchau Jul 31, 2019
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
85 changes: 61 additions & 24 deletions packages/gatsby-cli/src/reporter/reporters/ink/components/error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,40 @@ import path from "path"
import { Color, Box } from "ink"
import { get } from "lodash"

const colors = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll remove this -- I was testing with a structured "error" of warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

We need theme-ui theme for CLI!

Copy link
Contributor Author

@DSchau DSchau Jul 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

lol! Design tokens and theme-ui for our CLI, I like it!

WARNING: {
bg: { bgYellow: true, black: true },
fg: { yellow: true },
},
ERROR: {
bg: {
bgRed: true,
black: true,
},
fg: {
red: true,
},
},
INFO: {
bg: {
bgBlue: true,
white: true,
},
fg: {
blue: true,
},
},
DEBUG: {
bg: {
bgCyan: true,
black: true,
},
fg: {
cyan: true,
},
},
}

const File = ({ filePath, location }) => {
const lineNumber = get(location, `start.line`)

Expand Down Expand Up @@ -34,32 +68,34 @@ const DocsLink = ({ docsUrl }) => {
)
}

const Error = ({ details }) => (
// const stackLength = get(details, `stack.length`, 0

<Box marginY={1} flexDirection="column">
<Box flexDirection="column">
const Error = ({ details }) => {
const color = colors[details.level] || colors.error
return (
<Box marginY={1} flexDirection="column">
<Box flexDirection="column">
<Box>
<Box marginRight={1}>
<Color black bgRed>
{` ${details.level} `}
{details.id ? `#${details.id} ` : ``}
</Color>
<Color red>{details.type ? ` ` + details.type : ``}</Color>
<Box flexDirection="column">
<Box>
<Box marginRight={1}>
<Color {...color.bg}>
{` ${details.level} `}
{details.id ? `#${details.id} ` : ``}
</Color>
<Color {...color.fg}>
{details.type ? ` ` + details.type : ``}
</Color>
</Box>
</Box>
<Box marginTop={1}>{details.text}</Box>
{details.filePath && (
<Box marginTop={1}>
File:{` `}
<File filePath={details.filePath} location={details.location} />
</Box>
)}
</Box>
<Box marginTop={1}>{details.text}</Box>
{details.filePath && (
<Box marginTop={1}>
File:{` `}
<File filePath={details.filePath} location={details.location} />
</Box>
)}
<DocsLink docsUrl={details.docsUrl} />
</Box>
<DocsLink docsUrl={details.docsUrl} />
</Box>
{/* TODO: use this to replace errorFormatter.render in reporter.error func
{/* TODO: use this to replace errorFormatter.render in reporter.error func
{stackLength > 0 && (
<Box>
<Color>
Expand All @@ -74,7 +110,8 @@ const Error = ({ details }) => (
</Color>
</Box>
)} */}
</Box>
)
</Box>
)
}

export default Error
4 changes: 1 addition & 3 deletions packages/gatsby-cli/src/structured-errors/construct-error.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
24 changes: 16 additions & 8 deletions packages/gatsby/src/bootstrap/load-plugins/index.js
View file
Open in desktop
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