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

DSchau · 2019-07-26T00:23:41Z

Description

This WIP PR enables nicer error handling (structured errors!) for invalid APIs.

It does a few things (all of which I'll need to test more):

Uses a postinstall script to download latest-apis.json from Unpkg
Uses this API lookup to provide nicer errors/warnings
Improves (!?) the warning based on data from that API lookup
Does not panic/fail the build on these missing/typo'd API errors

Still to-do:

More manual testing
Unit and/or e2e testing
Validate postinstall script, and specifically error handling

Related Issues

Implements #16055

Helps unblock #16027

DSchau

Left some prelim. comments.

DSchau · 2019-07-26T00:24:28Z

packages/gatsby-cli/src/reporter/reporters/ink/components/error.js

@@ -3,6 +3,40 @@ import path from "path"
 import { Color, Box } from "ink"
 import { get } from "lodash"

+const colors = {


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

We need theme-ui theme for CLI!

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

DSchau · 2019-07-26T00:25:14Z

packages/gatsby/src/bootstrap/load-plugins/validate.js

 const { version: gatsbyVersion } = require(`gatsby/package.json`)
 const reporter = require(`gatsby-cli/lib/reporter`)
 const resolveModuleExports = require(`../resolve-module-exports`)

+const getGatsbyUpgradeVersion = entries =>


Gets the most recent/newest version of Gatsby if multiple new APIs.

DSchau · 2019-07-26T00:26:21Z

packages/gatsby/src/bootstrap/load-plugins/validate.js

- message += `\n\n`
- const similarities = stringSimiliarity.findBestMatch(bady.exportName, apis)
- const isDefaultPlugin = bady.pluginName == `default-site-plugin`
- const badExportsMigrationMap = {


I just removed these. Is there a reason to keep these around in Gatsby 2.x?

Ideally we don't remove those. It's definitely less likely that any user will hit that at this point in time (almost year after gatsby@2). Was this causing some headaches with refactoring?

Also in screenshot from PR description you had migration tips for those - was this just mockup?

Nope! That one (and maybe the other) get covered by the string distance algorithm!

DSchau · 2019-07-26T00:27:34Z

packages/gatsby-cli/src/structured-errors/error-map.js

@@ -160,6 +162,34 @@ const errorMap = {
 }`,
 level: `ERROR`,
 },
+ // invalid or deprecated APIs
+ "11329": {


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

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.

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

@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.

DSchau · 2019-07-26T00:35:22Z

packages/gatsby/src/utils/api-runner-node.js

- // to happen once the the API queue is empty. Ideally of course
- // we'd have an API (returning a promise) for that. But this
- // works nicely in the meantime.
- if (!apiList[api] && api !== `FAKE_API_CALL`) {


This was kind of a strange one -- I think I can safely remove this?

Maybe? See #11831 - FAKE_API_CALL was added as workaround for issues when page is deleted. I think you could try gatsby develop in gatsby-starter-blog and try to delete one of markdown posts and see what happens :P

pieh · 2019-07-26T00:55:56Z

packages/gatsby/src/utils/get-latest-apis.js

+ */
+ if (await fs.exists(OUTPUT_FILE)) {
+ return fs.readJSON(OUTPUT_FILE)
+ }


I think trying to load from file makes more sense as fallback and to prefer fetching fresh if possible.

In case of creating new gatsby project - cached latest-apis.json will be equaivalent to what gatsby package was doing already - as it will be list of APIs that is only up-to-date with their version of gatsby. If later on we add new API and implement it in plugin. Users install plugin - he still will have cached latest-apis.json that have no data about this new API

Yeah good call! Will fix.

sidharthachatterjee · 2019-07-29T23:29:58Z

packages/gatsby/src/utils/get-latest-apis.js

+const fs = require(`fs-extra`)
+const axios = require(`axios`)
+
+const API_FILE = `https://unpkg.com/gatsby/apis.json`


Nit: I'd put this behind a gatsby.dev url (gatsby.dev/apis.json) for future proofing and if we ever move this to something else

Yeah -- not sure. The main drawback is that sometimes .dev domains are blocked due to some localhosts hackery, which would mean the file never downloads.

I think keeping it like this is a little safer. I don't think it's likely we'll need to re-name this file, either?

I get what you're doing here, just not sure I'm fully on board!

Yeah, I'm likely overthinking this. The file isn't mission critical anyway and is a progressive enhancement so you're probably right in keeping things simple!

Looking great otherwise 🙌

It was (and still may be!) a good idea! Just want to try and think of all the trade offs, good and bad!

sidharthachatterjee

Time to ship this! Thank you so much @DSchau 👍

sidharthachatterjee · 2019-08-11T22:42:42Z

Published in gatsby@2.13.58

thchia · 2019-09-09T08:56:42Z

I haven’t grokked what’s going on here, but is it possible to opt out of downloading the latest apis?

My motivation is that this is running behind a firewall at my work network, and I have to wait for it to timeout which is a bit annoying. If there is a way to fast fail and go straight to reading the default apis.json file, that would do. Happy to contribute.

DSchau · 2019-09-09T14:50:57Z

@thchia the idea is that introducing this functionality enables us to keep errors/warnings on invalid APIs (e.g. typos or APIs we don't support) but not break on newer APIs that we release in newer versions of Gatsby, e.g. #16027 which introduces an API to validate/coerce the shape of plugin/theme options.

Could you share more info as to the work firewall? It seems like unpkg is an extremely safe candidate to not be blocked. Blocking that would block a significant chunk of the web.

What we could do is add a timeout (e.g. 2500 ms or something, but this is a "guess" that may then break slower connections unnecessarily) and fail if the timeout is exceeded -- but I'd like to hear more about your use case before we do that.

thchia · 2019-09-09T15:22:01Z

@DSchau thanks for the response.

I work at a financial institution, which has a security policy that is

Mysterious and;
Hard to change T_T

I’ve run into issues like this before (postinstall scripts that download from github also don’t work), and unfortunately for me the solution has never been to relook at the security policy. Packages like node-sass offer configurable URLs for downloading the necessary binaries, I’m not sure if you would consider this in the same bucket of issues?

I don’t think adding a timeout sounds like the right solution for the same reason you mentioned - it ends up being non-deterministic.

Anyway I’m aware that in any “normal” environment this wouldn’t be an issue and the problem is through no fault of this awesome tool 😃

DSchau added 5 commits July 25, 2019 11:38

feat(gatsby): cache latest APIs on Gatsby install

025874f

chore: slightly refactor with axios

c6864a7

feat(gatsby): introduced richer error logging for unknown APIs

f1ea99e

chore: warning -> error

b7a1930

chore: remove comment I used to test

7861d1f

DSchau added the status: WIP label Jul 26, 2019

DSchau commented Jul 26, 2019

View reviewed changes

fix: wrap postinstall in try/catch

74b4564

DSchau commented Jul 26, 2019

View reviewed changes

chore: slight tweaks to verbiage

a2dabd0

pieh reviewed Jul 26, 2019

View reviewed changes

DSchau added 3 commits July 29, 2019 15:55

fix: always prefer network

b4665f9

chore: reset changes

4c8a7d1

test: fix unit tests

3cbd0f0

sidharthachatterjee reviewed Jul 29, 2019

View reviewed changes

DSchau added 3 commits July 30, 2019 13:37

test: add a few more tests

bf55206

test: add some more tests

d4aa82d

test: fix windows for being a bozo

59bf7a3

DSchau marked this pull request as ready for review July 31, 2019 05:11

DSchau requested review from a team as code owners July 31, 2019 05:11

DSchau removed the status: WIP label Jul 31, 2019

DSchau changed the title ~~[WIP] feat(gatsby): enable richer error handling for unknown APIs~~ feat(gatsby): enable richer error handling for unknown APIs Jul 31, 2019

sidharthachatterjee approved these changes Aug 11, 2019

View reviewed changes

sidharthachatterjee merged commit 0adab13 into gatsbyjs:master Aug 11, 2019

DSchau mentioned this pull request Aug 12, 2019

feat: start work on validating plugin options #16027

Closed

5 tasks

DSchau deleted the gatsby/apis-list branch August 12, 2019 20:31

DSchau mentioned this pull request Sep 19, 2019

fix(gatsby): add timeout so fetching API info doesn't fail on very slow connections #17735

Merged

mxstbr mentioned this pull request Oct 14, 2020

feat(gatsby): release plugin option validation #27437

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

DSchau commented Jul 26, 2019 •

edited

Loading

DSchau left a comment

DSchau Jul 26, 2019

pieh Jul 26, 2019

DSchau Jul 26, 2019 •

edited

Loading

DSchau Jul 26, 2019

DSchau Jul 26, 2019

pieh Jul 26, 2019

pieh Jul 26, 2019

DSchau Jul 26, 2019

DSchau Jul 26, 2019

pieh Jul 26, 2019

DSchau Jul 26, 2019

DSchau Jul 29, 2019

DSchau Jul 26, 2019

pieh Jul 26, 2019

pieh Jul 26, 2019

DSchau Jul 26, 2019

DSchau Jul 29, 2019

sidharthachatterjee Jul 29, 2019

DSchau Jul 29, 2019

sidharthachatterjee Jul 29, 2019

DSchau Jul 30, 2019

sidharthachatterjee left a comment

sidharthachatterjee commented Aug 11, 2019

thchia commented Sep 9, 2019 •

edited

Loading

DSchau commented Sep 9, 2019

thchia commented Sep 9, 2019

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

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

Conversation

DSchau commented Jul 26, 2019 • edited Loading

Description

Related Issues

DSchau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DSchau Jul 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidharthachatterjee left a comment

Choose a reason for hiding this comment

sidharthachatterjee commented Aug 11, 2019

thchia commented Sep 9, 2019 • edited Loading

DSchau commented Sep 9, 2019

thchia commented Sep 9, 2019

DSchau commented Jul 26, 2019 •

edited

Loading

DSchau Jul 26, 2019 •

edited

Loading

thchia commented Sep 9, 2019 •

edited

Loading