feat(errors): add more verbose errors #1798

ctjlewis · 2021-01-24T00:44:11Z

Adding more verbose errors

This PR adds a class InternalCliError extends Error {} utility (which logs a stack trace and context object before throwing), and implements it in two locations.

Required fields

Summary

I tried to run netlify dev in the netlify/netlify-faunadb-example project and received a confusing error that prevented me from moving forward. I dug into the CLI source again and modified the error logic to be clearer (and I now more or less understand why the CLI was throwing). This class can be used throughout the codebase as desired for throwing unrecoverable runtime errors.

Test plan

This only modifies two existing errors at present, and should not require testing.

Changelog

Added clearer and more verbose error messages for a few edge cases.

Changes

Current behavior

In trying to use netlify dev with the netlify/netlify-faunadb-example project, I received the following error:

◈ Netlify Dev ◈
◈ Ignored general context env var: LANG (defined in process)
◈ Empty args assigned, this is an internal Netlify Dev bug, please report your settings and scripts so we can improve

This same error message would be provided in both cases below (no possibleArgsArrs, or netlify dev in a matching package.json script, as we see in netlify/netlify-faunadb-example).

Proposed changes

My philosophy here was that if the CLI fails, it should log as much information as possible. The signature of the constructor is InternalCliError(msg: string, context: object), and it will produce an Error with the same message as usual, but it will also use console.trace(...) to pretty-print the context object provided at throw-time.

With these changes merged, if no possible args array are found in scanScripts:

◈ Netlify Dev ◈
◈ Ignored general context env var: LANG (defined in process)
Trace: INTERNAL CLI ERROR. No possible args found.
Please open an issue at https://github.com/netlify/cli/issues/new and include the following information:
{
  "packageJsonScripts": {
    "docs": "md-magic --path '**/*.md' --ignore 'node_modules'",
    "prebuild": "echo 'setup faunaDB' && npm run bootstrap",
    "build": "react-scripts build"
  },
  "possibleArgsArrs": []
}

    at new InternalCliError (/home/christian/PersonalProjects/netlify-cli/src/utils/error.js:13:17)
    at chooseDefaultArgs (/home/christian/PersonalProjects/netlify-cli/src/utils/detect-server.js:247:11)
    at serverSettings (/home/christian/PersonalProjects/netlify-cli/src/utils/detect-server.js:49:23)
    at DevCommand.run (/home/christian/PersonalProjects/netlify-cli/src/commands/dev/index.js:234:24)
    at async DevCommand._run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/command.js:43:20)
    at async Config.runCommand (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/config/lib/config.js:173:24)
    at async Main.run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/main.js:27:9)
    at async Main._run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/command.js:43:20)
◈ No possible args found.

And for the netlify dev error (which presents when a detector matches a script that contains a call to netlify dev, to ensure it does not recursively call itself):

◈ Netlify Dev ◈
◈ Ignored general context env var: LANG (defined in process)
Trace: INTERNAL CLI ERROR. Cannot call `netlify dev` inside `netlify dev`.
Please open an issue at https://github.com/netlify/cli/issues/new and include the following information:
{
  "packageJsonScripts": {
    "bootstrap": "netlify dev:exec node ./scripts/bootstrap-fauna-database.js",
    "docs": "md-magic --path '**/*.md' --ignore 'node_modules'",
    "start": "netlify dev",
    "prebuild": "echo 'setup faunaDB' && npm run bootstrap",
    "build": "react-scripts build"
  }
}

    at new InternalCliError (/home/christian/PersonalProjects/netlify-cli/src/utils/error.js:13:17)
    at scanScripts (/home/christian/PersonalProjects/netlify-cli/src/detectors/utils/jsdetect.js:84:13)
    at detector (/home/christian/PersonalProjects/netlify-cli/src/detectors/create-react-app.js:17:28)
    at serverSettings (/home/christian/PersonalProjects/netlify-cli/src/utils/detect-server.js:43:30)
    at DevCommand.run (/home/christian/PersonalProjects/netlify-cli/src/commands/dev/index.js:234:24)
    at async DevCommand._run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/command.js:43:20)
    at async Config.runCommand (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/config/lib/config.js:173:24)
    at async Main.run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/main.js:27:9)
    at async Main._run (/home/christian/PersonalProjects/netlify-cli/node_modules/@oclif/command/lib/command.js:43:20)
◈ Cannot call `netlify dev` inside `netlify dev`.

Thrown errors throughout the CLI could theoretically be replaced pretty easily, and should help with bug reports as it will be much easier to copy-paste details.

A separate error was also added for the netlify dev in package.json issue, as it would just bubble up to the possibleArgsArr error before.

Footnotes

Super boring stylistic changes and comments were cleaned up, mostly just to save real estate and clarify the purpose of different statements. Some of the changes were required to silence the linter.

ctjlewis · 2021-01-24T00:46:18Z

~~Also, if anyone has any recommendations as to how to handle the `netlify dev` in package.json script for the FaunaDB example, I can add to that specific error message.~~

I actually do not know how I am supposed to run netlify dev in the FaunaDB example, given that the matching create-react-app script contains netlify dev and would cause the CLI to recursively call itself.

Edit

I just did not read the directions properly, and I was supposed to run npm run bootstrap which would interface with the CLI and set up the workspace. The CLI noticed the netlify dev call in the matching script and correctly threw an error to prevent recursive self-calling.

How about the following language:

`netlify dev` is attempting to call a package.json script that also calls `netlify dev`. Please run the script manually with `npm run [script]` instead of trying to call `netlify dev`.

It still seems odd that the FaunaDB example would embed a call to netlify dev in the start script, as this could conflict with CLI logic. Just thinking out loud.

The CLI detects different framework commands in package.json, and will throw if it detects `netlify dev` in the default script while running `netlify dev` in order to avoid recursively calling itself. By removing the `netlify dev` from the `start` and `dev` scripts and adjusting the CLI logic to recognize `netlify dev:exec` as distinct from `netlify:dev`, we can get this project working as expected. Also updates package.json dependencies. see also: netlify#26 netlify/cli#1798

see also: netlify/netlify-faunadb-example#36

ctjlewis · 2021-01-24T02:54:56Z

Distinguishing between netlify dev and netlify dev:* as per netlify/netlify-faunadb-example#36:

cli/src/detectors/utils/jsdetect.js

Lines 87 to 89 in 6d53bbf

    
           if (/netlify dev(?!:)/.test(scriptCommand)) { 
        
             throw new InternalCliError('Cannot call `netlify dev` inside `netlify dev`.', { packageJsonScripts }) 
        
           }

ctjlewis · 2021-01-24T03:04:13Z

Hate to CC you @lindsaylevine but you're quickly becoming my Netlify buddy, I hope it's not a bother to bug you for a quick review whenever you get a minute 😅

I promise to slow down on the PRs, I just send them in to fix stuff as I find it!

lindsaylevine · 2021-01-24T05:57:21Z

haha hey @ctjlewis!!!! no worries / no need to slow down as long as you can be patient with the team that owns the CLI (which, apologies, i am not on) 😁

ctjlewis · 2021-01-24T10:44:42Z

No worries Lindsay, my bad for tagging you then!

Also, after thinking about this some more, the "netlify dev tried to call netlify dev" situation should probably be a recoverable error, or at least, should not prompt the user to file an issue, because it's effectively expected behavior, and the user should just run yarn dev or whatever per the new error message instead of netlify dev (which will refuse to pick up on the package.json script and call itself recursively).

Will make those changes later today.

erezrokah

Hi @ctjlewis, you're on fire 🔥

I like the proposed changes in this PR (separating the errors and proper logging) but we have a PR that re-writes the way the current code works.

How about we incorporate those specific use cases into that PR?

Regarding error reporting, I wonder if logging inside the error ctor is the best approach.
We could use https://oclif.io/docs/error_handling#error-handling-in-the-catch-method to handle errors in a centralized location. WDYT?

erezrokah · 2021-01-28T14:49:31Z

I'll handle this as a part of #1704

feat(errors): add more verbose InternalCliError class with stack trace

2e06694

ctjlewis requested a review from a team as a code owner January 24, 2021 00:44

ctjlewis mentioned this pull request Jan 24, 2021

◈ Empty args assigned, this is an internal Netlify Dev bug, please report your settings and scripts so we can improve netlify/netlify-faunadb-example#26

Open

ctjlewis mentioned this pull request Jan 24, 2021

fix: prevent netlify dev from attempting to recursively call itself netlify/netlify-faunadb-example#36

Draft

fix: recognize netlify dev and netlify dev:exec as distinct commands

6d53bbf

see also: netlify/netlify-faunadb-example#36

chore: add more verbose error message

12b2200

ctjlewis changed the title ~~feat(errors): add more verbose InternalCliError class with stack trace~~ feat(errors): add more verbose errors Jan 24, 2021

erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jan 25, 2021

erezrokah self-requested a review January 25, 2021 15:56

erezrokah reviewed Jan 25, 2021

View reviewed changes

erezrokah closed this Jan 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(errors): add more verbose errors #1798

feat(errors): add more verbose errors #1798

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021

lindsaylevine commented Jan 24, 2021

ctjlewis commented Jan 24, 2021 •

edited

Loading

erezrokah left a comment

erezrokah commented Jan 28, 2021

feat(errors): add more verbose errors #1798

feat(errors): add more verbose errors #1798

Conversation

ctjlewis commented Jan 24, 2021 • edited Loading

Adding more verbose errors

Required fields

Summary

Test plan

Changelog

Changes

Current behavior

Proposed changes

Footnotes

ctjlewis commented Jan 24, 2021 • edited Loading

Edit

ctjlewis commented Jan 24, 2021 • edited Loading

ctjlewis commented Jan 24, 2021

lindsaylevine commented Jan 24, 2021

ctjlewis commented Jan 24, 2021 • edited Loading

erezrokah left a comment

Choose a reason for hiding this comment

erezrokah commented Jan 28, 2021

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021 •

edited

Loading

ctjlewis commented Jan 24, 2021 •

edited

Loading