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

wp-env: no longer show error message twice #20157

Merged
merged 2 commits into from Feb 13, 2020
Merged

Conversation

@noahtallen
Copy link
Contributor

noahtallen commented Feb 10, 2020

I noticed during some testing today that wp-env would show error messages twice. This PR attempts to only display the error message once. @noisysocks / @epiqueras I may be missing the context for why we needed to check for error instanceof env.ValidationError and then display the error twice, so if you have thoughts I'm all ears. :) Perhaps there are some cases where multiline error output wouldn't be shown by spinner.fail?

Screenshots

Screen Shot 2020-02-10 at 2 21 57 PM

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 10, 2020

@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 10, 2020

Makes sense! Done :)

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Feb 10, 2020

@noahtallen noahtallen force-pushed the fix/wp-env-errors-twice branch from 44f5e3c to 400f22b Feb 12, 2020
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 12, 2020

Rebased. cc @noisysocks again just in case. ;)

@noisysocks

This comment has been minimized.

Copy link
Member

noisysocks commented Feb 13, 2020

I think we still want to print error.err to STDERR?

This part of the code confused me too. There's many types of error and so writing one handler for them all involves considering several execution paths 😅

I'm thinking let's give up and just explicitly handle each of the different error types instead:

( error ) => {
	if ( error instanceof env.ValidationError ) {
		// Error is a validation error. That means the user did something wrong.
		spinner.fail( error.message );
		process.exit( 1 );
	} else if (
		'exitCode' in error &&
		'err' in error &&
		'out' in error
	) {
		// Error is a docker-compose error. That means something docker-related failed.
		// https://github.com/PDMLab/docker-compose/blob/master/src/index.ts
		spinner.fail( 'Error while running docker-compose command.' );
		if ( error.out ) {
			process.stdout.write( error.out );
		}
		if ( error.err ) {
			process.stderr.write( error.err );
		}
		process.exit( error.exitCode );
	} else {
		// Error is an unknown error. That means there was a bug in our code.
		spinner.fail( error.message );
		// Disable reason: Using console.error() means we get a stack trace.
		// eslint-disable-next-line no-console
		console.error( error );
		process.exit( 1 );
	}
}
- Separate cases for errors we know about
- More straightforward general error case
@noahtallen noahtallen force-pushed the fix/wp-env-errors-twice branch from 400f22b to aaafd07 Feb 13, 2020
@noahtallen

This comment has been minimized.

Copy link
Contributor Author

noahtallen commented Feb 13, 2020

@noisysocks That makes a lot of sense, done!

Updated with the new code, the same error looks like this, which is definitely an improvement.

Screen Shot 2020-02-12 at 7 14 39 PM

@noisysocks noisysocks merged commit 7891953 into master Feb 13, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@noisysocks noisysocks deleted the fix/wp-env-errors-twice branch Feb 13, 2020
@github-actions github-actions bot added this to the Gutenberg 7.6 milestone Feb 13, 2020
jorgefilipecosta added a commit that referenced this pull request Mar 2, 2020
* Handle errors more robustly

- Separate cases for errors we know about
- More straightforward general error case

* Update tests to handle the new error format
jorgefilipecosta added a commit that referenced this pull request Mar 2, 2020
* Handle errors more robustly

- Separate cases for errors we know about
- More straightforward general error case

* Update tests to handle the new error format
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.