Dev server improvements #762

drwpow · 2020-08-07T18:02:53Z

Changes

This improves the dev server logging. Now:

✅ All console.* messages log and keep (even across page loads!)
✅ Plugins can throw and the output will bubble up to the dev server
✅ Dev server shows state as ERROR or READY, along with error log (error persists even after fix)

Screenshots

Default (no errors)

React

Vue

Svelte

Lit Element

How about the status bar at bottom? Any changes needed there?

Testing

vercel · 2020-08-07T18:02:57Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/pikapkg/snowpack/btpz1ahx3
✅ Preview: https://snowpack-git-dev-dashboard-2.pikapkg.vercel.app

drwpow · 2020-08-12T00:58:40Z

packages/snowpack/src/commands/dev.ts

@@ -275,6 +236,12 @@ export async function command(commandOptions: CommandOptions) {
    await updateLockfileHash(DEV_DEPENDENCIES_DIR);
  }

+  // Start painting dev dashboard after successful install


One annoying thing: we still have a pino instance in install.ts, which means any install errors get wiped away when the dev server starts (because it logs to stderr, not console.error, so changing that means changing all pino instances too). We don’t throw an error in install, currently. And we don’t allow passing a logger in that function.

What should we do here? Should we throw an error in install.ts? Should we add dev vs build logic in install to handle different logging ability (pino vs messageBus)? Unclear what the simplest way would be to take an install error and pass it to the dev server’s array of logs.

@FredKSchott

drwpow · 2020-08-12T00:59:01Z

packages/snowpack/src/commands/dev.ts

-        currentlyRunningCommand = null;
-      });
-    }
-  }


No more auto-install on dev! 🎉

drwpow · 2020-08-12T00:59:37Z

packages/@snowpack/plugin-vue/plugin.js

      if (errors && errors.length > 0) {
-        console.error(JSON.stringify(errors));
+        throw new Error(displayError({error: errors[0], contents, filePath}));


This error display for Vue was real bad, so I added one (see screenshots)

stramel · 2020-08-12T02:00:23Z

Wow! Huge improvement! I'm going to try to pull it down tomorrow and play around with it. How does the proxy output look? Might be nice to have that listed below the main line?

stramel · 2020-08-12T03:56:24Z

My previous csa generated app just sits at loading...

FredKSchott

This is already a huge improvement!

I have some basic design feedback of the output, but if it's alright with you I'd love to play around with the PR a bit to test some ideas before making any suggestions.

FredKSchott · 2020-08-12T15:55:13Z

packages/snowpack/src/build/build-pipeline.ts

@@ -95,12 +102,22 @@ async function runPipelineLoadStep(
          // if source maps disabled, don’t return any
          if (!sourceMaps) result[ext].map = undefined;
        });
+        if (devMessageBus && isDev) {
+          devMessageBus.emit('SUCCESS', {id: step.name});


already talked through this in person, but this is the bit that isn't really compatible with our current idea of a whole plugin b eing SUCCESS or FAIL.

example 1:

App.svelte loads in the dev server, has an error, fails

Icon.svelte also loads, succeeds, "SUCCESS" overwrites "ERROR"

End result is no error shown

example 2:

App.svelte loads in the dev server, has an error, fails

"Svelte" plugin is now in the "error" state

What does it mean to go back to the "success" state? Do we need to track which files failed, and then only go to "SUCCESS" state when that file succeeds? what if the file is deleted? etc etc

Right. I thought of that convo while doing this, and I agree it’s a bit hacky. But one thing I realized in playing with the dev server we didn’t discuss was it felt “broken” in a sense to show the READY status if the dev server was running but there was an error and nothing was showing. It felt a lot better to either show READY or ERROR based on the state, but to your point this isn’t a perfect implementation.

Maybe we change that READY status to RUNNING? Or something else? Or maybe we just take the green color away? Open to a solution where an error is visible and it doesn’t feel like the dev server is “lying” to us telling us everything is good when it’s not.

FredKSchott · 2020-08-12T22:49:27Z

Made some changes! Will let the screenshots show:

Startup, Simple Project (Preact), No Workers

`load()` Error, Simple Project (Preact), No Workers

Startup, Complex Project (Svelte + TypeScript)

Errors, Complex Project (Svelte + TypeScript)

White Color Scheme

stramel · 2020-08-13T01:44:28Z

Seems like the latest batch of changes fixed the issue with it not starting.

One thing I noticed which is probably unrelated to these changes, but missing files are a bit annoying.

Would still love to see proxy get some love with these changes but I can submit a PR after the fact if you want.

Currently, it just logs to console which is now greatly improved to where we can at least see the errors.

It would be nice to see something like this (example of Next.js):

stramel · 2020-08-13T02:28:56Z

packages/snowpack/src/commands/paint.ts

+};
+
+const MAX_CONSOLE_LENGTH = 500;
+const NO_COLOR_ENABLED = process.env.FORCE_COLOR === '0' || process.env.NO_COLOR;


I don't know if this is necessary unless we want to support NO_COLOR which isn't supported by kleur yet. I opened a PR to address this, lukeed/kleur#37

If we want to support NO_COLOR before this, we should be able to do colors.$.enabled = !process.env.NO_COLOR && colors.$.enabled. This should allow all colors.* calls to do nothing. https://github.com/lukeed/kleur#conditional-support

Just kidding, Luke is on top of it!

https://github.com/lukeed/kleur/releases/tag/v4.1.0

Now we shouldn't need to add anything to support disabling colors.

Nice! Upstream contributions, ftw!

FredKSchott · 2020-08-13T05:10:03Z

+1 for better proxy output, can take a look at that similar to Next.js

Not sure what the service worker issue is but most likely not related to this PR, would love more info if you want to spin up an issue/discussion to debug/triage

stramel · 2020-08-13T15:26:22Z

Not sure what the service worker issue is but most likely not related to this PR, would love more info if you want to spin up an issue/discussion to debug/triage

I previously had a service worker registered on that localhost:port, so when I started the app it requested a /service-worker.js file. I just thought it was a lot of output for a missing file. Is there a way we could condense the fact that we're checking different file extensions? maybe like service-worker.[js|mjs|jsx|ts|tsx]?

I just pulled in the latest and ran it again, seems like something changed? Might be an issue with linking? I know at one point it wasn't adding the absolute path

Also, just curious if it would make sense to make paths relative to the running directory instead of absolute? For example,

/Users/mstramel/Projects/testing/snowpack-tests/test-1/public/service-worker.js

to

/public/service-worker.js

since the project is being ran out of /Users/mstramel/Projects/testing/snowpack-tests/test-1

FredKSchott · 2020-08-13T16:28:05Z

Lets keep output as is for now, but would be curious to clean up in a follow up PR. I have definitely made mistakes mapping a relative path to an absolute path in my head (when a tool doesn't give you the full absolute path) so I'm sensitive to it as a debugging tool.

@stramel I'm confused why the plugin name is that full path in your logs, are you on the latest version of the babel plugin? If updating doesn't fix it, can you share your config file? That's definitely a bug

stramel · 2020-08-13T16:46:45Z

@stramel I'm confused why the plugin name is that full path in your logs, are you on the latest version of the babel plugin? If updating doesn't fix it, can you share your config file? That's definitely a bug

It went away after I updated my dependencies. 👍

drwpow force-pushed the dev-dashboard-2 branch from 1733a33 to 691d27b Compare August 11, 2020 18:09

vercel bot deployed to Preview August 11, 2020 18:09 View deployment

drwpow force-pushed the dev-dashboard-2 branch from 691d27b to 1013f18 Compare August 11, 2020 19:43

vercel bot deployed to Preview August 11, 2020 19:43 View deployment

drwpow force-pushed the dev-dashboard-2 branch from 1013f18 to 4cff6bf Compare August 12, 2020 00:28

vercel bot deployed to Preview August 12, 2020 00:28 View deployment

drwpow marked this pull request as ready for review August 12, 2020 00:28

drwpow requested a review from a team as a code owner August 12, 2020 00:28

drwpow changed the title ~~[wip] Dev server improvements~~ Dev server improvements Aug 12, 2020

drwpow requested a review from FredKSchott August 12, 2020 00:31

drwpow force-pushed the dev-dashboard-2 branch from 4cff6bf to 82262df Compare August 12, 2020 00:34

vercel bot deployed to Preview August 12, 2020 00:34 View deployment

drwpow force-pushed the dev-dashboard-2 branch from 82262df to 7068602 Compare August 12, 2020 00:36

vercel bot deployed to Preview August 12, 2020 00:37 View deployment

drwpow force-pushed the dev-dashboard-2 branch from 7068602 to cbbbcc8 Compare August 12, 2020 00:55

vercel bot deployed to Preview August 12, 2020 00:55 View deployment

drwpow commented Aug 12, 2020

View reviewed changes

drwpow force-pushed the dev-dashboard-2 branch from cbbbcc8 to bfaa126 Compare August 12, 2020 02:31

vercel bot deployed to Preview August 12, 2020 02:31 View deployment

drwpow force-pushed the dev-dashboard-2 branch from bfaa126 to 78698f0 Compare August 12, 2020 02:35

vercel bot deployed to Preview August 12, 2020 02:35 View deployment

FredKSchott reviewed Aug 12, 2020

View reviewed changes

vercel bot deployed to Preview August 12, 2020 22:45 View deployment

vercel bot deployed to Preview August 12, 2020 23:07 View deployment

stramel reviewed Aug 13, 2020

View reviewed changes

vercel bot deployed to Preview August 13, 2020 05:14 View deployment

FredKSchott mentioned this pull request Aug 13, 2020

build --watch mode #782

Merged

drwpow and others added 5 commits August 13, 2020 14:43

Improve dev server output

a7e3cc1

fred attempt

334fe7c

fix bad spacing on http url line of header

f9f71c3

add proxy message

c55a56a

add build watch message similar to dev

e16b0ed

FredKSchott force-pushed the dev-dashboard-2 branch from f90f648 to e16b0ed Compare August 13, 2020 21:45

vercel bot deployed to Preview August 13, 2020 21:45 View deployment

updates

c5acbad

vercel bot deployed to Preview August 13, 2020 21:47 View deployment

FredKSchott merged commit 610fb9d into master Aug 13, 2020

FredKSchott deleted the dev-dashboard-2 branch August 13, 2020 21:53

FredKSchott mentioned this pull request Aug 14, 2020

[Enhancement] Output Improvements #648

Closed

7 tasks

joaotavora mentioned this pull request Dec 22, 2020

Support for npm/yarn linked package updates #429

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dev server improvements #762

Dev server improvements #762

drwpow commented Aug 7, 2020 •

edited

Loading

vercel bot commented Aug 7, 2020 •

edited

Loading

drwpow Aug 12, 2020 •

edited

Loading

drwpow Aug 12, 2020

drwpow Aug 12, 2020

drwpow Aug 12, 2020 •

edited

Loading

stramel commented Aug 12, 2020

stramel commented Aug 12, 2020

FredKSchott left a comment

FredKSchott Aug 12, 2020

drwpow Aug 12, 2020 •

edited

Loading

FredKSchott commented Aug 12, 2020

stramel commented Aug 13, 2020

stramel Aug 13, 2020 •

edited

Loading

stramel Aug 13, 2020

FredKSchott Aug 13, 2020 •

edited

Loading

FredKSchott commented Aug 13, 2020

stramel commented Aug 13, 2020

FredKSchott commented Aug 13, 2020

stramel commented Aug 13, 2020

Dev server improvements #762

Dev server improvements #762

Conversation

drwpow commented Aug 7, 2020 • edited Loading

Changes

Screenshots

Default (no errors)

React

Vue

Svelte

Lit Element

Testing

vercel bot commented Aug 7, 2020 • edited Loading

drwpow Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

drwpow Aug 12, 2020

Choose a reason for hiding this comment

drwpow Aug 12, 2020

Choose a reason for hiding this comment

drwpow Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

stramel commented Aug 12, 2020

stramel commented Aug 12, 2020

FredKSchott left a comment

Choose a reason for hiding this comment

FredKSchott Aug 12, 2020

Choose a reason for hiding this comment

drwpow Aug 12, 2020 • edited Loading

Choose a reason for hiding this comment

FredKSchott commented Aug 12, 2020

Startup, Simple Project (Preact), No Workers

load() Error, Simple Project (Preact), No Workers

Startup, Complex Project (Svelte + TypeScript)

Errors, Complex Project (Svelte + TypeScript)

White Color Scheme

stramel commented Aug 13, 2020

stramel Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

stramel Aug 13, 2020

Choose a reason for hiding this comment

FredKSchott Aug 13, 2020 • edited Loading

Choose a reason for hiding this comment

FredKSchott commented Aug 13, 2020

stramel commented Aug 13, 2020

FredKSchott commented Aug 13, 2020

stramel commented Aug 13, 2020

drwpow commented Aug 7, 2020 •

edited

Loading

vercel bot commented Aug 7, 2020 •

edited

Loading

drwpow Aug 12, 2020 •

edited

Loading

drwpow Aug 12, 2020 •

edited

Loading

drwpow Aug 12, 2020 •

edited

Loading

`load()` Error, Simple Project (Preact), No Workers

stramel Aug 13, 2020 •

edited

Loading

FredKSchott Aug 13, 2020 •

edited

Loading