feat: better error messages with human readable outputs #609

thecodrr · 2019-08-03T05:12:33Z

Summary:

This is a work in progress and might take more than a little time.

These checks should add a little more context (and actionable output) when the script fails and reduce the number of false issues. All these handle potential points of failure e.g. invalid arguments, typos in provided file/dir paths or directories that no longer exist.

Adding a list here of all the commands that need to checked.

CLI

Android

log-android [no need]
run-android [done but not tested]
link [no need]
config [no need]

Test Plan:

Only way to test is by failing all the checks one by one. 😆 Maybe more jest tests can be added to assure these comply?

thymikee

Really nice work here, appreciate it! Here's some early feedback

packages/cli/src/commands/init/template.js

packages/cli/src/commands/bundle/saveAssets.js

packages/cli/src/commands/init/templateName.js

packages/platform-android/src/commands/runAndroid/index.ts

thecodrr · 2019-08-05T05:27:08Z

@thymikee Thank you!

A question: I have not been able to test the platform-android code. Is there a simple way I can run it and check the error outputs?

thymikee · 2019-08-05T05:59:40Z

You need to yarn link @react-native-community/cli-platform-android in the test project

thecodrr · 2019-08-06T05:10:08Z

@thymikee I am done with this, please review.

grabbou

I like the direction this PR is moving. Is it already ready to be reviewed or merged? I left few comments and I am wondering what would be the next step now (it's been a while)

packages/cli/src/commands/bundle/assetPathUtils.js

packages/cli/src/commands/bundle/saveAssets.js

thecodrr · 2019-08-23T05:32:28Z

@grabbou thanks. Yes this is done and ready to be reviewed. I may have left some errors out (intentionally) because they don't make sense or slow down the performance.

grabbou · 2019-10-01T14:05:23Z

Uhm, looks like we have moved on with Typescript and other PRs in the meantime (unintentionally), making conflicts with this PR.

I do really like it and was wondering if you'd be open to rebasing and preparing it for merging once again?

thecodrr · 2019-10-05T07:19:30Z

@grabbou Okay I will begin the work ASAP.

thymikee · 2019-10-11T14:08:27Z

@thecodrr I took a stab at it and rebased your work. Please review if I didn't mess anything up :)

thecodrr · 2019-10-20T10:22:13Z

@thymikee Everything looks perfect. : )

thecodrr · 2019-11-27T09:44:13Z

@thymikee ping

Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>

grabbou

Great stuff! Did you like CLIError? I spent a bit of time working on this abstraction and I am happy that you found it useful here!

* refactor: properly handle invalid template package * refactor: error checks when getting template name * refactor: remove duplicate exception throw * refactor: check if assets dest exists * refactor: add error checks when upgrading * chore: add comment for using process.exit() * refactor: improve error outputs in link command * refactor: use for of loop * refactor: check package before uninstalling * refactor: use CLIError instead of direct exit * refactor: handle potential android pkg name errors * refactor: fix typos/mistakes in error outputs * refactor: add adb & android sdk path checks * refactor: fix imports * refactor: improve install apk error outputs * Apply suggestions from code review Co-Authored-By: Michał Pierzchała <thymikee@gmail.com> * refactor: add init doc link in error output * test: make sure all tests pass * refactor: use try/catch around readFileSync * refactor: do not read build.gradle twice * refactor: clean up code in android linking * fix: packageJson not defined in templateName * refactor: remove unintentional --verbose flag * refactor: make flow very happy * refactor: remove unnecessary variable definition * pkgjson undefined tweak * eslint fix * remove adb/android_home checks because we have doctor * use mockImplementationOnce * revert link changes, as it's going to be removed in next version * inline fns * revert uninstall changes as we're removing that in next version * adjust copy * mock existsSync once * remove unused code Co-authored-by: Michał Pierzchała <thymikee@gmail.com>

adrianha · 2020-03-23T06:19:39Z

packages/cli/src/commands/bundle/saveAssets.ts

    return Promise.resolve();
  }

+  if (!fs.existsSync(assetsDest)) {


hi @thecodrr @thymikee @grabbou,
I have a situation that I need to run bundle and copy assets into new created folder based on assetsDest and I was relying on https://github.com/react-native-community/cli/pull/609/files#diff-e59437386ac414095e6ed43a3da37303R93 to create folder if not exists.
Any specific reason that it needs to check assetsDest first before copying assets?

Since we are calling mkdirp in the copy function, maybe this is no longer needed?

yes, i think it should be safe using mkdirp before copy files

this check can be safely removed then. What do you think @thymikee?

Thanks @thecodrr @thymikee will open the PR

thecodrr requested review from Esemesek, grabbou and thymikee as code owners August 3, 2019 05:12

thymikee reviewed Aug 4, 2019

View reviewed changes

thecodrr mentioned this pull request Aug 5, 2019

Rethink automatic discovery of commands #568

Open

thecodrr changed the title ~~WIP: Add proper error checks with human readable outputs~~ Add proper error checks with human readable outputs Aug 6, 2019

grabbou reviewed Aug 21, 2019

View reviewed changes

packages/cli/src/commands/bundle/assetPathUtils.js Show resolved Hide resolved

packages/cli/src/commands/bundle/saveAssets.js Outdated Show resolved Hide resolved

thymikee force-pushed the properly-handle-template-errors branch from 3cb73bc to 06b9cec Compare October 11, 2019 14:07

thymikee changed the title ~~Add proper error checks with human readable outputs~~ feat: better error messages with human readable outputs Oct 11, 2019

thecodrr added 12 commits November 28, 2019 16:48

refactor: properly handle invalid template package

f40353f

refactor: error checks when getting template name

710d8fb

refactor: remove duplicate exception throw

1c69278

refactor: check if assets dest exists

30b0698

refactor: add error checks when upgrading

e27b60b

chore: add comment for using process.exit()

9f6cea1

refactor: improve error outputs in link command

1d4b52c

refactor: use for of loop

b44a7e8

refactor: check package before uninstalling

c4f1f46

refactor: use CLIError instead of direct exit

bc6c633

refactor: handle potential android pkg name errors

d6f5657

refactor: fix typos/mistakes in error outputs

a69ee5b

thecodrr and others added 15 commits November 28, 2019 16:48

refactor: add adb & android sdk path checks

f321431

refactor: fix imports

389e60f

refactor: improve install apk error outputs

8a138ce

Apply suggestions from code review

207ea95

Co-Authored-By: Michał Pierzchała <thymikee@gmail.com>

refactor: add init doc link in error output

6bf9dba

test: make sure all tests pass

168d679

refactor: use try/catch around readFileSync

d851843

refactor: do not read build.gradle twice

c0a2cac

refactor: clean up code in android linking

d073a04

fix: packageJson not defined in templateName

9087af4

refactor: remove unintentional --verbose flag

fbf5bd8

refactor: make flow very happy

af2b0cc

refactor: remove unnecessary variable definition

f2f6e08

pkgjson undefined tweak

415bb25

eslint fix

879f9b8

thymikee force-pushed the properly-handle-template-errors branch from 06b9cec to 879f9b8 Compare November 28, 2019 16:00

thymikee added 7 commits November 28, 2019 17:21

remove adb/android_home checks because we have doctor

e87dc75

use mockImplementationOnce

1e4ccb1

revert link changes, as it's going to be removed in next version

a3b88f4

inline fns

3091c13

revert uninstall changes as we're removing that in next version

8b23e14

adjust copy

37d4466

mock existsSync once

ea4ffd8

Esemesek approved these changes Nov 29, 2019

View reviewed changes

remove unused code

398e7c4

grabbou approved these changes Jan 30, 2020

View reviewed changes

grabbou merged commit 2e3302e into react-native-community:master Jan 30, 2020

adrianha reviewed Mar 23, 2020

View reviewed changes

adrianha mentioned this pull request Mar 23, 2020

feat: remove assetsDest checking #1064

Merged

feat: better error messages with human readable outputs #609

feat: better error messages with human readable outputs #609

Uh oh!

Conversation

thecodrr commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary:

CLI

Android

Test Plan:

Uh oh!

thymikee left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thecodrr commented Aug 5, 2019

Uh oh!

thymikee commented Aug 5, 2019

Uh oh!

thecodrr commented Aug 6, 2019

Uh oh!

grabbou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thecodrr commented Aug 23, 2019

Uh oh!

grabbou commented Oct 1, 2019

Uh oh!

thecodrr commented Oct 5, 2019

Uh oh!

thymikee commented Oct 11, 2019

Uh oh!

thecodrr commented Oct 20, 2019

Uh oh!

thecodrr commented Nov 27, 2019

Uh oh!

grabbou left a comment

Choose a reason for hiding this comment

Uh oh!

adrianha Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

thecodrr Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

adrianha Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

thecodrr Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

thymikee Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

adrianha Mar 23, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

thecodrr commented Aug 3, 2019 •

edited

Loading