search

diegodurli / flow-status-webpack-plugin

Run Flow Status on each Webpack build.
MIT License
98 stars 16 forks source link

Using notifications through node-notifier #14

Closed prewk closed 8 years ago

prewk commented 8 years ago

This was useful to me, might be useful to someone else as well.

It doesn't create any dependencies on node-notifier or anything, it's very optional.

diegodurli commented 8 years ago

Hey @prewk!

Thank you for this PR, loved the idea!

However, don't you think would be useful to have a success message too? Mainly to sinalize a recovery from errors

I guess would be nice to know it's all fine after a few errors being recovered. What you think?

prewk commented 8 years ago

Yeah! I think perhaps an onError and onSuccess callback instead of a "notifier" might be the most flexible. That way you can notify however you like in your webpack config instead of depending node-notifier specifically. Fallback = the console.log that's in the code today.

diegodurli commented 8 years ago

It's more flexible, indeed @prewk. Do you want to go ahead and submit a PR following this?

prewk commented 8 years ago

Sure thing. tis 16 aug. 2016 kl. 20:21 skrev Diego Durli notifications@github.com:

It's more flexible, indeed @prewk https://github.com/prewk. Do you want to go ahead and submit a PR following this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/diegodurli/flow-status-webpack-plugin/pull/14#issuecomment-240191488, or mute the thread https://github.com/notifications/unsubscribe-auth/AAnEZro-Jo0B_2l85l6-T4XtZ7mCgRR9ks5qgf-hgaJpZM4JjwF2 .

prewk commented 8 years ago

I added some tests as well for added coziness.

Are you going to merge pull request #12? In that case do that first, and I'll change this according to it. It might be cumbersome for the other author otherwise.

diegodurli commented 8 years ago

Hi @prewk!

I just merge that PR, so I guess would be nice for you to update yours. Sorry about that @prewk!

Thank you for such a great feature! 👍

diegodurli commented 8 years ago

@prewk, I'm just updating the package patch version now and publishing to NPM. Going to update master to reflect it now. Just to let you know.

prewk commented 8 years ago

@diegodurli np I'm syncing atm. I was wondering though, what's the purpose of this?

function isFunction(x) {
  return Object.prototype.toString.call(x) == '[object Function]';
}

instead of just typeof x === 'function'?

diegodurli commented 8 years ago

Agreed @prewk, nowadays its not necessary anymore to have this test, typeof is enough for this purpose. We can start testing it directly.

diegodurli commented 8 years ago

@prewk this one can be closed then? I just merged your newest PR.

prewk commented 8 years ago

Yup