Log content paths to console #167

tjallingt · 2018-06-07T10:18:12Z

This PR contains a:

Motivation / Use-Case

This is an attempt to add the logging and documentation improvements described in #160

I ran into this behaviour after switching to webpack-serve from a webpack + static file server setup. I didn't remove my static javascript files previously outputted by webpack and this caused webpack-serve to not give me my updated files (but since the files still existed everything seemed to work 😕).

Hopefully this note will allow people in the future to more easily realize that this is the default behaviour.

Breaking Changes

N/A

Additional Info

I accidentaly ran prettier on the readme.md but the changes seemed good so I just stuck with it. (sorry for the random changes is what I'm trying to say).

tjallingt · 2018-06-07T10:21:21Z

I personally think the console info are a bit hard to see because by default webpack-serve logs a lot of information. Therefore I'm not so sure it is a good/useful change (then again I don't think it hurts).

I tried to stick as much as possible to the description of the logging here: #160 (comment)

shellscape

Appreciate the effort here, but I got about halfway through the changeset and there are so many changes related to your (or your editor's) personal style preferences that I had to stop.

Please don't make changes that are personal style preferences.

You'll need to revert all of the stylistic changes to blocked-code and markdown before we can move forward with review.

shellscape · 2018-06-07T13:15:47Z

README.md

- in your package.json file in a `serve` property
- in a `.serverc` or `.serverc.json` file, in either JSON or YML.
- in a `serve.config.js` file which exports a CommonJS module (just like webpack).
+* in your package.json file in a `serve` property


unnecessary change here, isn't it?

shellscape · 2018-06-07T13:16:16Z

README.md

-[Events](#events).
+* `close()` _(Function)_ - Closes the server and its dependencies.
+* `on(eventName, fn)` _(Function)_ - Binds a serve event to a function. See
+  [Events](#events).


unneeded extra spaces

shellscape · 2018-06-07T13:16:39Z

README.md

- `close()` *(Function)* - Closes the server and its dependencies.
- `on(eventName, fn)` *(Function)* - Binds a serve event to a function. See
-[Events](#events).
+* `close()` _(Function)_ - Closes the server and its dependencies.


Again, there's no need for these changes. Please don't apply personal/editor preferences in PRs

shellscape · 2018-06-07T13:17:31Z

README.md

-The path, or array of paths, from which content will be served.
+The path, or array of paths, from which static content will be served.
+
+_Note: by default the files served from the `content` paths take precedence over files generated by webpack._


Please be aware of and adjust for the line boundaries here. You'll notice all other lines in the file are :)

shellscape · 2018-06-07T13:20:40Z

README.md

-  'warn',
-  'error'
-]
+['trace', 'debug', 'info', 'warn', 'error'];


Please don't make changes that are personal style preferences.

shellscape · 2018-06-07T13:21:18Z

README.md

@@ -336,7 +343,7 @@ For example:
 const serve = require('webpack-serve');
 const config = require('./webpack.config.js');

-serve({ config }).then((server) => {
+serve({ config }).then(server => {


Please don't make changes that are personal style preferences. This change would be invalid with our linter in this repo.

I accidentaly ran prettier on the readme.md but the changes seemed good so I just stuck with it. (sorry for the random changes is what I'm trying to say).

Maybe (especially since i mentioned this in my pull request) there is a nicer way to phrase this and not spam me with the same message about something that is not my personal preference at all (as a matter of fact I didn't realize it happened until I started writing the pr message).

Maybe (especially since i mentioned this in my pull request) there is a nicer way to phrase this and not spam me with the same message about something that is not my personal preference at all

That's a bit too sensitive there on your part. If you have your editor (or prettier in this case) setup with a specific configuration that runs contrary to the style already present in a repo, those are indeed personal style preferences. Also note my overall review comment. I had not realized just how many changes of this nature existed until halfway down the review, at which point I stopped. I'm not sure how much experience you have in reviews on high-volume orgs/projects, but users tend to miss associated changes unless you comment on every. single. one. of them. So never be upset that a reviewer is being thorough.

shellscape · 2018-06-07T13:21:33Z

README.md

-Arguments:  
-  [`Compiler`](https://webpack.js.org/api/node/#compiler-instance) _compiler_
+Arguments:
+


Please don't make changes that are personal style preferences.

shellscape · 2018-06-07T13:21:39Z

README.md

-  [`Stats`](https://webpack.js.org/api/node/#stats-object) _stats_  
-  [`Compiler`](https://webpack.js.org/api/node/#compiler-instance) _compiler_
+Arguments:
+


Please don't make changes that are personal style preferences.

shellscape · 2018-06-07T13:21:44Z

README.md

-  [`Stats`](https://webpack.js.org/api/node/#stats-tojson-options-) _json_
-  [`Compiler`](https://webpack.js.org/api/node/#compiler-instance) _compiler_
+Arguments:
+


Please don't make changes that are personal style preferences.

This reverts commit 2240e91.

This reverts commit 49dfcf6.

codecov · 2018-06-07T14:03:55Z

Codecov Report

Merging #167 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   98.21%   98.27%   +0.06%     
==========================================
  Files           7        7              
  Lines         280      290      +10     
==========================================
+ Hits          275      285      +10     
  Misses          5        5

Impacted Files	Coverage Δ
lib/server.js	`97.05% <100%> (+0.5%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 45ececd...965c75c. Read the comment docs.

shellscape · 2018-06-07T14:05:56Z

reverted formatting, i said i was sorry dude chill

Please refrain from commit messages like this in webpack-contrib org projects. It's just not needed. If you start to feel that a review is coming after you personally, then it's a great opportunity to step back and take a few minutes, and then return when it can be looked at objectively.

tjallingt · 2018-06-07T14:11:31Z

I was just joking... 🤷‍♂️

If the commit message is a problem, just squash it when merging 👍

shellscape · 2018-06-07T14:07:49Z

lib/server.js

@@ -13,7 +14,7 @@ const weblog = require('webpack-log');

 const WebpackServeError = require('./WebpackServeError');

-module.exports = (options) => {
+module.exports = options => {


this will fail linting, as will several other changes in this file. run npm run lint before the next commit.

yea I tried running the linter but its giving me about 2000 failures:

/* many more like this */ 50:1 error Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style 51:45 error Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style 52:1 error Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style 53:11 error Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style 54:3 error Expected linebreaks to be 'LF' but found 'CRLF' linebreak-style ✖ 2273 problems (2273 errors, 0 warnings) 2273 errors, 0 warnings potentially fixable with the `--fix` option.

that looks like you have an environment (editor, etc) issue on your end there. perhaps your editor isn't recognizing or obeying the linting setup for the repo?

I think git is messing with me since the files are indeed crlf (both my editor and eslint agree on that) formatted but git doesn't seem to think its a problem (unless i convert everything in which case it tells me im changing every file in the repo)

shellscape · 2018-06-07T14:13:00Z

lib/server.js

@@ -1,3 +1,4 @@
+const path = require('path');


since the relative function is the only one being used on path, please change this to:

const { relative } = require('path');

and update the line using it below

shellscape · 2018-06-07T14:18:57Z

lib/server.js

+        log.info(chalk`Serving Static Content from: {grey ${paths[0]}}`);
+      } else {
+        log.info('Serving Static Content from:');
+        paths.forEach(path => log.info(chalk.grey(`\t- ${path}`)));


(not sure why this one didn't make it into the previous review, I blame github)

we tend not to use tabs in console output, since their display from one OS to another can be messy and they tend not to line up consistently. for those reasons, please use spaces here, and please do omit the hyphen. using log.info for each path is going to produce the log info prefix for every line, which we also don't want to do here.

The end result should look like:

i ｢serve｣: Serving Static Content from: /some/path/first /some/path/second /some/path/third

we also shy away from forEach as for/of is more performant after Node v6.

Oh I didn't know for of was faster good to know 👍
You are right as it is now it is not neatly logging out the paths, i'm on it!

@frezzy

* refactor: initial refactor * chore: refactor flags, add tests * chore: config tests * test: config, options tests. snapshot updates * test: compound flag to object tests * chore: aw man this is some dope refactor * test: lib/compiler tests * test: middleware tests * test: lib/server tests * test: lib/app tests * test: lib/index tests * test: 100% coverage 💥👊👊👊👊💥 * chore: update use of cli-utils * chore: reapply fix from #182 * test: silent the bus test * fix: fix flag group parsing, hot-client flag schema * chore: export flag schema as js object for formatting * test: add --help and --version cli tests * chore: improve help output * test: add https get test * chore: apply fixes from #167 * chore: add cli-utils dependency * docs: update docs with breaking changes. type fix from #178 * test: update tests and snaps for cross-node-version CI * test: more changes for node 6 test compat * test: skip tests we dont want to run for node 6 * test: re-add removed snapshots for node > 6 * test: update compiler tests * chore: update cli-utils, cli test snapshots * chore: update cli schema and help * docs: update readme with latest cli help * test: increase timeout for node 6 + cli tests * test: update cli test port to avoid conflict in node 8 ci * docs: update content option with precedence explanation * chore: make suggested meta changes * fix: addresses #160. prefer webpack bundle files first. previously, static files defined in 'content' path(s) would take precedence, being served over bundle files in a webpack-dev-middleware MemoryFileSystem with the same path. This has been reversed, giving webpack bundles precedence. * chore: beta2 * chore: validate the type of the args parameter * chore(release): beta3 * fix: alphabetize content directories * fix: only push cwd to content if no context * fix: call middleware in series and wait for server * test: update tests + snapshots * test: tweak coverage and make instanbul behave * docs: add docs for listening event back in * chore: update manual middleware call pattern * test: add 'add' test for fallthrough. credit @frezzy * docs: fix typo * docs: fix proxy-router example mistake * chore: beta4 * docs: add webpack-serve-overlay by @G-Rath to 3rd party addons * docs: add note to content about paths * fix: make another pass at fixing https websockets * chore: beta 5 * test: update snapshots * test: fix middleware snapshots * test: use --detectOpenHandles for silly node 6 * chore: revert --detectOpenHandles * chore: tracking down node v6 ci problem * chore: see what happens with node 6 removed * chore: ci madness * chore: beta 6 * chore: update koa-webpack to non-beta * chore: update lockfile

tjallingt added 3 commits June 7, 2018 11:52

added logging of statically served folders

66d13a4

added some more information to the readme

49dfcf6

for consistency

2240e91

tjallingt changed the title ~~Pr/log static path~~ Log content paths to console Jun 7, 2018

shellscape suggested changes Jun 7, 2018

View reviewed changes

tjallingt added 2 commits June 7, 2018 16:03

Revert "for consistency"

fd1c6fd

This reverts commit 2240e91.

Revert "added some more information to the readme"

6bc1258

This reverts commit 49dfcf6.

reverted formatting, i said i was sorry dude chill

3d468c1

fixed line boundary

8721f42

shellscape suggested changes Jun 7, 2018

View reviewed changes

shellscape reviewed Jun 7, 2018

View reviewed changes

tjallingt and others added 2 commits June 7, 2018 16:43

fix linting errors and better formatting of the logged message

a682349

docs: improve content precedence note, code

965c75c

shellscape approved these changes Jun 20, 2018

View reviewed changes

shellscape merged commit bae2209 into webpack-contrib:master Jun 20, 2018

shellscape mentioned this pull request Jun 20, 2018

Proposal to improve developer experience around the static file serving performance optimisation #160

Closed

3 tasks

shellscape added a commit that referenced this pull request Jun 20, 2018

chore: apply fixes from #167

08642b0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Log content paths to console #167

Log content paths to console #167

tjallingt commented Jun 7, 2018 •

edited by shellscape

Loading

tjallingt commented Jun 7, 2018

shellscape left a comment

shellscape Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

tjallingt Jun 7, 2018

shellscape Jun 7, 2018 •

edited

Loading

shellscape Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

codecov bot commented Jun 7, 2018 •

edited

Loading

shellscape commented Jun 7, 2018

tjallingt commented Jun 7, 2018 •

edited

Loading

shellscape Jun 7, 2018

tjallingt Jun 7, 2018

shellscape Jun 7, 2018

tjallingt Jun 7, 2018

shellscape Jun 7, 2018

shellscape Jun 7, 2018

tjallingt Jun 7, 2018

Log content paths to console #167

Log content paths to console #167

Conversation

tjallingt commented Jun 7, 2018 • edited by shellscape Loading

Motivation / Use-Case

Breaking Changes

Additional Info

tjallingt commented Jun 7, 2018

shellscape left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shellscape Jun 7, 2018 • 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

codecov bot commented Jun 7, 2018 • edited Loading

Codecov Report

shellscape commented Jun 7, 2018

tjallingt commented Jun 7, 2018 • 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

Choose a reason for hiding this comment

tjallingt commented Jun 7, 2018 •

edited by shellscape

Loading

shellscape Jun 7, 2018 •

edited

Loading

codecov bot commented Jun 7, 2018 •

edited

Loading

tjallingt commented Jun 7, 2018 •

edited

Loading