RoccoC / webpack-build-notifier

A Webpack plugin that generates OS notifications for build steps using node-notifier.
MIT License
162 stars 24 forks source link

Fix incorrect `messageFormatter` type #72

Closed astorije closed 3 years ago

astorije commented 3 years ago

Fixes https://github.com/RoccoC/webpack-build-notifier/pull/68

It looks like I messed up a rebase or something. Sorry!

(It would be nice if the types were not defined twice as I've hit this a couple times in my PRs. I can take a stab at consolidating them from the Config object if you're interested)

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 155


Totals Coverage Status
Change from base Build 151: 0.0%
Covered Lines: 130
Relevant Lines: 134

💛 - Coveralls
RoccoC commented 3 years ago

Whoops! I missed it, too.

Yeah, would be better if that type wasn't defined twice. I'll merge your PR and will consolidate the types.

RoccoC commented 3 years ago

Done: ba29e0dca2b341cc65fa541a45d435fe4f877776

Although it would probably have been cleaner to just collapse all of the individual class properties into one config object, e.g.:

before:

export default class WebpackBuildNotifierPlugin {
  // config options
  private title: string = 'Webpack Build';
  private logo?: string;
  private sound: string = 'Submarine';
  private successSound: string;
  private warningSound: string;
  ...

after:

export default class WebpackBuildNotifierPlugin {
  private config: Config;
...

Maybe an improvement for another time. :)

RoccoC commented 3 years ago

Thanks for the fix!