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

feat(karma-webpack): unify webpack and karma color config #356

Merged
merged 2 commits into from
Nov 28, 2018

Conversation

spicalous
Copy link
Contributor

@spicalous spicalous commented Sep 11, 2018

Unifies the color config between webpack and Karma. See #332

Type

  • Feature

Issues

SemVer

  • Minor

@jsf-clabot
Copy link

jsf-clabot commented Sep 11, 2018

CLA assistant check
All committers have signed the CLA.

@michael-ciniawsky michael-ciniawsky changed the title feat(karma-webpack): unify webpack and Karma colour config feat(karma-webpack): unify webpack and karma color config Sep 14, 2018
@michael-ciniawsky michael-ciniawsky modified the milestones: 3.1.0, 4.0.0 Sep 14, 2018

// Set webpack's color config to value specified in Karma's config for consistency
webpackMiddlewareOptions.stats = webpackMiddlewareOptions.stats || {};
webpackMiddlewareOptions.stats.colors = colors;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this always override the default webpack colors config ? This would be a breaking change then... Could this eventually be made an option intead ? e.g

if (config.colors) {
  webpackDevMiddlewareOptions.stats.colors = colors
}

⚠️ Not sure if this works correctly, but to illustrate the idea :)

Copy link
Contributor Author

@spicalous spicalous Sep 14, 2018

Choose a reason for hiding this comment

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

@michael-ciniawsky Thanks for the response :) What do you mean by a breaking change?

It should work in a similar way that webpack's watch config is being overridden by Karma's singleRun config https://github.com/webpack-contrib/karma-webpack/blob/master/src/karma-webpack.js#L56

If you specify colors: false on Karma, you probably don't want colors on webpack's output either

Copy link
Contributor

@michael-ciniawsky michael-ciniawsky Sep 14, 2018

Choose a reason for hiding this comment

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

I'm honstely not too familiar with in this particular area, so I might misunderstand the changes made here... My current understanding is that webpackDevMiddlewareOptions.stats.colors = colors always 'overrides' the webpack defaults (?) What are the webpack defaults? No colors at all (then everthing should be fine as-is)? Could you maybe post a screenshot before/after to clearify?

Copy link
Contributor Author

@spicalous spicalous Sep 15, 2018

Choose a reason for hiding this comment

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

Below are some images to illustrate the changes

I have a simple project setup you can play around with https://github.com/spicalous/color-example

First image: No configs (Karma and webpack output color)
default-no-color-opt

Second image: Karma color:false (Karma outputs no color, webpack outpus color)
karma-color-false

Third image: Karma color:false after changes (Karma and webpack both output no color)
karma-color-false-after

Choose a reason for hiding this comment

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

webpackMiddlewareOptions.stats could be string. This breaks configuration i.e.:

  //...
  stats: 'errors-only'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed in rc.5

Copy link
Collaborator

@ryanclark ryanclark left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants