fix: handle env and stage errors cleanly. Closes #1942 #1943

zetlen · 2019-10-29T21:32:09Z

Description

Add error checks for all usages of loadEnvironment()
Add better error handling for Buildpack CLI and venia-concept staging server

Related Issue

Closes #1942.

Acceptance

Verification Stakeholders

@dpatil-magento

Verification Steps

Verify that #1942 is no longer reproducible when the environment is invalid.

Checklist

I have updated the documentation accordingly, if necessary.
I have added tests to cover my changes, if necessary.

PWAStudioBot · 2019-10-29T21:35:38Z

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against 1db0b5e

sirugh · 2019-10-31T14:24:06Z

packages/venia-concept/package.json

@@ -29,7 +29,7 @@
    "storybook:build": "yarn run storybook",
    "test": "yarn run -s prettier:check && yarn run -s lint && jest",
    "validate-queries": "yarn run download-schema && graphql validate-magento-pwa-queries --project venia",
-    "watch": "webpack-dev-server --progress --color --env.mode development"
+    "watch": "buildpack load-env . && webpack-dev-server --progress --color --env.mode development"


This is the key here, right? We now validate the environment before running the dev server. We should add this to start:debug and to the watch:all script.

I don't know if it belongs in start:debug, but it definitely belongs in watch:all. Good looking out.

sirugh · 2019-10-31T14:27:56Z

packages/venia-concept/server.js

@@ -11,6 +11,11 @@ const path = require('path');
 async function serve() {
    const config = loadEnvironment(__dirname);

+    if (config.error) {
+        // loadEnvironment takes care of logging it
+        process.exit(1);


To test this I had to do:

❯ DEV_SERVER_PORT=bad yarn run stage:venia

Just FYI for other verifiers.

Yeah, sorry, I didn't copy the verification steps over from #1942.

sirugh · 2019-10-31T14:28:53Z

packages/pwa-buildpack/bin/buildpack

-            throw err;
-        }
+        const error = err || msg;
+        prettyLogger.error(error.toString());


How do I test this? I commented out this code and nothing happened.

It's a fallback error handler for an unrecognized command. If you comment it out and run buildpack with no arguments, you'll see the difference. It belongs in this ticket because it's about handling errors cleanly, and this fallback error handler will handle all errors in upcoming versions of yargs.

fix: watch:all and handler comment

sirugh

You did make changes! Thanks :)

dpatil-magento · 2019-11-05T18:22:20Z

@zetlen bash docker/run-docker failing.

Works on develop :

Erron on zetlen/fix-loadenv-1942 :

dpatil-magento · 2019-11-05T21:28:49Z

QA Pass, @sirugh and/or @jimbo Can be merged once you approve latest commit.

sirugh · 2019-11-05T21:33:36Z

package.json

@@ -31,6 +31,7 @@
    "prepare": "node scripts/monorepo-introduction.js",
    "prettier": "prettier --loglevel warn --write '@(packages|scripts)/**/*.@(css|graphql|js)' '*.js'",
    "prettier:check": "prettier --list-different '@(packages|scripts)/**/*.@(css|graphql|js)' '*.js'",
+    "prevenia": "node ./packages/pwa-buildpack/bin/buildpack load-env ./packages/venia-concept",


What is prevenia?

It runs before yarn run venia. I'm just using the NPM scripts pre feature here to make the code a little easier to read. But I guess it didn't work on you. Should I just put this in venia instead?

Oh interesting. So since venia is just shorthand for yarn workspace @magento/venia-concept, this script runs every time yarn venia is invoked.

This is clever, but it may result in some surprising outcomes. If someone doesn't know about your venia shorthand, and tries to do a standard thing such as yarn workspace @magento/venia-concept run validate-queries, they'll miss out on load-env and probably be confused about why it's not working.

At least if all of that is in venia, they'll immediately see what the problem was.

Fair enough. The rest of our scripts are littered with shell &&s, so that's what this will be too.

dpatil-magento · 2019-11-07T19:00:29Z

re-verified after recent commits, looks good.

zetlen added tooling Related to the developer tooling, including buildpack, webpack plugins, and debug UIs. pkg:pwa-buildpack labels Oct 29, 2019

PWAStudioBot added the pkg:venia-concept label Oct 29, 2019

zetlen added the version: Patch This changeset includes backwards compatible bug fixes. label Oct 30, 2019

zetlen force-pushed the zetlen/fix-loadenv-1942 branch from 6d590d1 to 31e6200 Compare October 30, 2019 21:10

sirugh requested changes Oct 31, 2019

View reviewed changes

m2-community-project bot assigned sirugh Oct 31, 2019

m2-community-project bot added the Progress: changes requested label Oct 31, 2019

m2-community-project bot assigned zetlen Oct 31, 2019

m2-community-project bot added Progress: review and removed Progress: changes requested labels Oct 31, 2019

fix: handle env and stage errors cleanly. Closes #1942

fe2bc57

fix: watch:all and handler comment

zetlen force-pushed the zetlen/fix-loadenv-1942 branch from 282a44a to fe2bc57 Compare October 31, 2019 17:14

zetlen requested a review from sirugh October 31, 2019 17:14

Merge branch 'develop' into zetlen/fix-loadenv-1942

5bba031

m2-community-project bot added Progress: changes requested and removed Progress: review labels Nov 4, 2019

sirugh previously approved these changes Nov 4, 2019

View reviewed changes

jimbo previously approved these changes Nov 4, 2019

View reviewed changes

m2-community-project bot assigned jimbo Nov 4, 2019

m2-community-project bot added Progress: approved and removed Progress: changes requested labels Nov 4, 2019

Merge branch 'develop' into zetlen/fix-loadenv-1942

cab2612

m2-community-project bot removed the Progress: approved label Nov 5, 2019

fix: move load-env calls to root and clean up venia

da5cd9e

zetlen dismissed stale reviews from jimbo and sirugh via da5cd9e November 5, 2019 21:02

sirugh reviewed Nov 5, 2019

View reviewed changes

dpatil-magento and others added 3 commits November 6, 2019 12:29

Merge branch 'develop' into zetlen/fix-loadenv-1942

4dec4db

Merge branch 'develop' into zetlen/fix-loadenv-1942

c620421

fixup switch to shell logic over npm script logic

1db0b5e

jimbo approved these changes Nov 7, 2019

View reviewed changes

sirugh approved these changes Nov 7, 2019

View reviewed changes

dpatil-magento merged commit 8ff974d into develop Nov 7, 2019

dpatil-magento deleted the zetlen/fix-loadenv-1942 branch September 14, 2020 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle env and stage errors cleanly. Closes #1942 #1943

fix: handle env and stage errors cleanly. Closes #1942 #1943

zetlen commented Oct 29, 2019

PWAStudioBot commented Oct 29, 2019 •

edited

Loading

sirugh Oct 31, 2019

zetlen Oct 31, 2019

sirugh Oct 31, 2019

zetlen Oct 31, 2019

sirugh Oct 31, 2019

zetlen Oct 31, 2019

sirugh left a comment

dpatil-magento commented Nov 5, 2019

dpatil-magento commented Nov 5, 2019

sirugh Nov 5, 2019

zetlen Nov 6, 2019

jimbo Nov 6, 2019

zetlen Nov 7, 2019

dpatil-magento commented Nov 7, 2019

fix: handle env and stage errors cleanly. Closes #1942 #1943

fix: handle env and stage errors cleanly. Closes #1942 #1943

Conversation

zetlen commented Oct 29, 2019

Description

Related Issue

Acceptance

Verification Stakeholders

Verification Steps

Checklist

PWAStudioBot commented Oct 29, 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

sirugh left a comment

Choose a reason for hiding this comment

dpatil-magento commented Nov 5, 2019

dpatil-magento commented Nov 5, 2019

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

dpatil-magento commented Nov 7, 2019

PWAStudioBot commented Oct 29, 2019 •

edited

Loading