Skip to content
This repository has been archived by the owner on Apr 8, 2019. It is now read-only.

Optimize compilation events to prevent unneeded stats.toJson() call #182

Merged
merged 7 commits into from
Jun 20, 2018
Merged

Optimize compilation events to prevent unneeded stats.toJson() call #182

merged 7 commits into from
Jun 20, 2018

Conversation

MLoughry
Copy link
Contributor

@MLoughry MLoughry commented Jun 19, 2018

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Fixes #181 to prevent unneeded calls to stats.toJson()

Breaking Changes

None

Additional Info

None

@jsf-clabot
Copy link

jsf-clabot commented Jun 19, 2018

CLA assistant check
All committers have signed the CLA.

@shellscape
Copy link
Contributor

@MLoughry please don't remove portions of issue/pull request templates. we'll review once your pull request template is complete, and your changes are updated to pass CI.

Per nanobus documentation:
> ### When shouldn't I use this package?
> If you're only writing code that runs inside Node and don't need a '*' listener, consider using the built-in event emitter API instead.
@MLoughry
Copy link
Contributor Author

I updated to fix tests by changing away from using nanobus to using Node's native EventEmitter.

Per nanobus's documentation:

When shouldn't I use this package?

If you're only writing code that runs inside Node and don't need a '*' listener, consider using the built-in event emitter API instead.

I also added the parts of the bug template back that I had deleted, even though I deleted them precisely because there was nothing to say in those sections.

@codecov
Copy link

codecov bot commented Jun 19, 2018

Codecov Report

Merging #182 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage   98.23%   98.25%   +0.01%     
==========================================
  Files           7        7              
  Lines         283      286       +3     
==========================================
+ Hits          278      281       +3     
  Misses          5        5
Impacted Files Coverage Δ
index.js 100% <100%> (ø) ⬆️

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 012ab60...58b01bf. Read the comment docs.

@shellscape
Copy link
Contributor

Thanks for adding the template back, and for the PR 🍺

I also added the parts of the bug template back that I had deleted, even though I deleted them precisely because there was nothing to say in those sections.

They aren't there to pick and choose. None or n/a is acceptable. You'll find that for most projects that provide templates, removing portions of them is frowned upon. Templates are there to make the lives of maintainers and passers-by a little bit easier.

I updated to fix tests by changing away from using nanobus to using Node's native EventEmitter.

That's a major bummer. We won't be able to approve this PR with that change.

Copy link
Contributor

@shellscape shellscape left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't commit package-lock files in PRs. the maintainers will update the lockfiles as needed.

lib/bus.js Outdated
@@ -1,12 +1,12 @@
const isPlainObject = require('lodash/isPlainObject');
const nanobus = require('nanobus');
const EventEmitter = require('events');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please revert this back to nanobus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to include comments as to why nanobus is being used, when even their documentation would indicate that this project should use the native EventEmitter.

Regardless, nanobus luckily does have a listeners() API, so I'm able to work around the issue.

index.js Outdated

if (stats.hasWarnings()) {
bus.emit('compiler-warning', { json, compiler: comp });
const compilerErrorEventName = 'compiler-error';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is very microsofty :)

would prefer:

const events = {
  error: 'compiler-error',
  warning: 'compiler-warning',
};

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Some tests are timing out locally, but I can't figure out why. Maybe it's just an issue on my box?
@MLoughry
Copy link
Contributor Author

I've addressed both comments from the previous review.

note: the event name object const(s) really weren't needed as the event
names only appear twice each.
@shellscape
Copy link
Contributor

@MLoughry looks good. I made some slight alterations with a commit. Please let me know how you feel about it. (I'm bouncing between contexts like a madman today and it was quicker to sprinkle my sugar on it than ask you for another change)

@MLoughry
Copy link
Contributor Author

It looks fine.

@shellscape shellscape merged commit 416f414 into webpack-contrib:master Jun 20, 2018
shellscape added a commit that referenced this pull request Jun 20, 2018
shellscape added a commit that referenced this pull request Jul 9, 2018
* 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

webpack-serve does not scale to large projects
3 participants