Skip to content

Include ember-cli-build-notifications by default#328

Closed
mattmcmanus wants to merge 3 commits intoemberjs:masterfrom
mattmcmanus:master
Closed

Include ember-cli-build-notifications by default#328
mattmcmanus wants to merge 3 commits intoemberjs:masterfrom
mattmcmanus:master

Conversation

@mattmcmanus
Copy link

@rtablada
Copy link
Contributor

I think system level notifications could be really helpful for developers.

Coming from a project where builds take about 1-2 seconds on incremental builds and initial loads take a while, I usually know when I have triggered a new build.

But working on some smaller 3.0+ projects the build times and refreshes are often completed before I am in the browser, this can lead to sometimes not realizing the refresh has even occurred.
A system notification could help this.


Playing devil's advocate the notification queue on Mac can get really cluttered really quickly since it keeps a backlog of notifications.
I haven't worked with ember-cli-build-notifications to know if it pollutes this too badly, but I have run into issues with other test/build notifiers that would fill up the queue and make it basically unusable for finding other application messages.


## Summary

Adding ember-cli-build-notifications as a default addon will increase developer productivity and improve the feedback cycle of day-to-day development. It will also assist new ember devs by quickly notifying them of errors they do not yet know to look at for. The default settings should should be verbose, as that would be most helpful to new Ember developers.
Copy link
Contributor

Choose a reason for hiding this comment

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

One "should" should be enough 😉

Copy link
Author

Choose a reason for hiding this comment

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

😊Thanks.

Build notifications give automatic insight into these scenarios in an unobtrusive way.

## Detailed design
1. Include `ember-cli-build-notifications` as part of the default kit
Copy link
Contributor

Choose a reason for hiding this comment

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

For people like me, who didn't know of this addon before, it would be helpful to have a short summary what it actually does, so we don't have to look for the github page.

Also maybe elaborate how it would solve the scenarios you described in the previous section?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I added some more info!

@mattmcmanus
Copy link
Author

Playing devil's advocate the notification queue on Mac can get really cluttered really quickly since it keeps a backlog of notifications.

@rtablada Good point. I turned that off rather quickly because it is noisy. I added a note in drawbacks.

@patocallaghan
Copy link

While I think this will be a good change, similar to others concerns, I can definitely see myself changing the config to only turn it on for error builds. I only want to be notified when there’s a problem, it may become quite noisy otherwise.

I see ember-cli-build-notification’s default is not to turn this on for successful builds. Is there any reason not to keep this behaviour here too?

@mattmcmanus
Copy link
Author

@patocallaghan I tried to lay out my thoughts on why Success notifications are helpful in the scenarios above. We've been using the addon at Yapp for a while now. Over time, I've turned on the success messages for nearly all of our internal apps and addons. It eliminates so many possible questions about the status of the build. I'm also a person who generally disables notifications because I find them really distracting. That's never really been the case with this addon.

I suppose it's worth trying to make some distinctions. The apps we're working on are BIG. The incremental rebuild can take several seconds and it's really easy to disengage. The notification, whether success or error, is always an actionable thing for me.

@patocallaghan
Copy link

Fair enough, I guess it's hard to gauge until I've tried it. I'm just worried that if they are too noisy they'll cause people to have a negative experience. Anyhoo I'll defer to your judgement as you've been using it a lot. 👍

@mehulkar
Copy link

mehulkar commented May 3, 2018

I’m not familiar with this addon, but one thing to consider is that it’s very likely that two people on the same team will disagree with the config for this. I would be much more happier about this being included by default if I could control the setting for myself locally (and not using system/OS settings). Some kind of environment variable config or a local rc file that we can gitignore would be ++.

@sandstrom
Copy link
Contributor

sandstrom commented May 3, 2018

Neat functionality! 🏆

My three thoughts:

  1. Notification should only be shown for errors by default.
  2. There must be an easy way (dropping file in ~/.ember-cli or similar) of setting this per user (overriding project defaults). Clear instructions on how this can be configured in ~/.ember-cli. For a team of 10+ people it's almost guaranteed that there will be different preferences on notification behaviour.
  3. Ideally they shouldn't be kept in the macOS notification center history (too much noise, not relevant with a log in this case).

@stefanpenner
Copy link
Member

stefanpenner commented May 3, 2018

There must be an easy way (dropping file in ~/.ember-cli-rc or similar) of setting this per user (overriding project defaults). For a team of 10+ people it's almost guaranteed that there will be different preferences on notification behaviour.

+1

Ideally they shouldn't be kept in the macOS notification center history (too much noise, not relevant with a log in this case).

+1

@mattmcmanus
Copy link
Author

@mehulkar @sandstrom @patocallaghan @stefanpenner Could I impose on you to give the addon a try for a couple days with the settings I propose? The real test will be when you turn it off after the couple days is whether you miss the notifications. That's how I really knew I liked the setup is because whenever I'm on a project that doesn't have it I feel blind.

There must be an easy way (dropping file in ~/.ember-cli-rc or similar) of setting this per user (overriding project defaults). For a team of 10+ people it's almost guaranteed that there will be different preferences on notification behaviour.

As far as I'm aware, this isn't functionality that ember-cli currently has, right? That could be really useful.

Ideally they shouldn't be kept in the macOS notification center history (too much noise, not relevant with a log in this case).

I agree and I'm exploring whether this is an option.

@rwjblue
Copy link
Member

rwjblue commented Jun 15, 2018

@mehulkar @sandstrom @patocallaghan @stefanpenner - Did y'all give this a try? How did that pan out?

I'm personally keen to move forward here as I think that the overall ergonomics are much better, but I definitely want to make sure we have investigated various options / concerns / etc...

@mattmcmanus
Copy link
Author

A nice status update: @pdud (the developer of the addon) just released an update to the addon that puts all notifications in a single group. So it will never take more than on item in notification center.

@sandstrom
Copy link
Contributor

@rwjblue I haven't tried it. I have no new comments besides the three I've outlined above.

@rwjblue
Copy link
Member

rwjblue commented Jun 17, 2018

@sandstrom - Thank you for cross linking!

Notification should only be shown for errors by default.

This would also have been my default preference. I am not massively opposed to having it enabled all the time, but I'm not sure that notifications for success is as helpful as notifications of failures...

There must be an easy way (dropping file in ~/.ember-cli-rc or similar) of setting this per user (overriding project defaults). For a team of 10+ people it's almost guaranteed that there will be different preferences on notification behaviour.

Totally agreed! ember-cli has long needed this capability. We basically need one more type of config file (that is not intended to be checked in) in the project for personal per-project + per-user preferences, and this seems like as good a time as any to build that in. Just spit-ballin' here, but .ember-cli would continue to be the main checked-in project level config file and we would add a .ember-cli.local (or some other better name ;p ) that would be excluded from git...

Ideally they shouldn't be kept in the macOS notification center history (too much noise, not relevant with a log in this case).

Agreed. I believe that the most recent update to the addon makes this really nice, thank you for pointing it out!

@stefanpenner
Copy link
Member

stefanpenner commented Jun 18, 2018

There must be an easy way (dropping file in ~/.ember-cli-rc or similar)

Isn't this already a thing? my ~/.ember-cli file contains:

cat ~/.ember-cli 
{
  "yarn": true
}

and works like a charm.

Maybe some merging isn't working as expected?

@stefanpenner
Copy link
Member

@rwjblue this works for me, i am 👍

@sandstrom
Copy link
Contributor

@stefanpenner Yep, you are right, I didn't know that file already existed.

@rwjblue
Copy link
Member

rwjblue commented Jun 18, 2018

@stefanpenner - We currently have (listed in order of precedence):

  • per-user config (~/.ember-cli)
  • per-project config (<project-root>/.ember-cli)

I was suggesting adding another to that list, for user-specific overrides of project config, which would be .gitignored by default.

This would allow projects to have good default/general settings, but also allow users to override those for their own personal preference. A couple of examples where this would be really nice:

  • setting a default --test-port / --port for a specific project (allows users running many projects at once to still run ember s instead of ember s -p 9999).
  • allowing customizing launchers with ember test --server (some folks may want --no-launch, others only want --launch="firefox", etc)

Regardless, of this added level of configuration, I think this RFC is still a good idea...

@patocallaghan
Copy link

I did try out the addon but unfortunately I haven’t been doing much feature work so I couldn’t really make a proper assessment of it.

I think the least controversial option to move the RFC along would be to introduce it with only error notifications enabled. The behaviour to turn it on all the time could be revisited if we see many people adopt that usage in the wild.

I also didn’t know about the ~/.ember-cli settings override so that could probably be enough to introduce it and allow users to configure it personally. The per-project user config, while nice, could probably be done in another RFC rather than block this one.

@stefanpenner
Copy link
Member

stefanpenner commented Jun 18, 2018

I was suggesting adding another to that list, for user-specific overrides of project config, which would be .gitignored by default.

ah ok, sounds good.

@locks
Copy link
Contributor

locks commented Nov 13, 2020

We discussed this RFC at the last Framework team meeting and decided to initiate the final comment period to Close this RFC. Since this proposal was first introduced there have developments both in Ember (build error page) and the general ecosystem which makes this proposal not as relevant.

@locks locks added the FCP to close The core-team that owns this RFC has moved that this RFC be closed. label Nov 13, 2020
@rwjblue rwjblue closed this Nov 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

FCP to close The core-team that owns this RFC has moved that this RFC be closed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants