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

Add option to skip notifications on first build. #21

Merged
merged 1 commit into from
Dec 15, 2016

Conversation

etlovett
Copy link
Contributor

With complex build configs, you can end up with lots of notifications on the initial build, which can be annoying, so it's convenient to have the option to skip them on the initial build and only get them on incremental builds (when using alwaysNotify: true). It was sufficiently trivial to add it that I decided to open a full PR rather than ask about it via an issue first.

I've tried to match the style of the existing code, but let me know if there's something you'd like tweaked. I'm also open to bikeshedding on the option name, if you have something else you'd prefer.

@Turbo87
Copy link
Owner

Turbo87 commented Dec 15, 2016

@etlovett I'm wondering if that should actually be the default behavior...

@etlovett
Copy link
Contributor Author

@Turbo87 In my opinion that comes down to whether most projects that use this have simple or complex build configurations. I like being notified on initial build for a simple configuration that produces one or maybe two notifications, but the flurry of notifications gets annoying with more complex configurations that build 5+ packages. I would assume that most users (particularly those that prefer the out-of-the-box experience) have relatively simple build configurations, and so therefore leaving notifications on by default is better.

That said, I'm happy to make the switch if you like.

@Turbo87
Copy link
Owner

Turbo87 commented Dec 15, 2016

problem is that I've stopped using webpack quite some time ago so it's hard for me to judge what's best anymore...

@etlovett
Copy link
Contributor Author

Alright, so how would you like to proceed? I'd choose landing this as-is because it aligns with my (data-limited) assumptions about Webpack usage and doesn't require reworking things. I'm happy to change it though if you'd prefer to do so before merging.

@Turbo87 Turbo87 merged commit f4960f3 into Turbo87:master Dec 15, 2016
@Turbo87
Copy link
Owner

Turbo87 commented Dec 15, 2016

@etlovett done, thanks!

@etlovett
Copy link
Contributor Author

@Turbo87 Great, thanks! What's the path to getting this change published in npm, and is there anything I can do to help? I'd love to be able to use this flag in my project.

@Turbo87
Copy link
Owner

Turbo87 commented Dec 15, 2016

@etlovett should be published as v1.5.0 now

@etlovett etlovett deleted the skip-first branch December 16, 2016 01:19
@etlovett
Copy link
Contributor Author

@Turbo87 Excellent, thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants