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

Show system notifications for errors #131

Closed
wants to merge 1 commit into from

Conversation

goshacmd
Copy link
Contributor

@goshacmd goshacmd commented Jul 23, 2016

Uses webpack-notifier under the hood. No setup necessary.
Supports Notification Center on OS X, Toaster on Windows 8 and later.
Falls back to Growl if present.

(https://github.com/mikaelbr/node-notifier/blob/master/DECISION_FLOW.md)

Ref #89.

Notifications look like this:

screen shot 2016-07-23 at 12 42 54 pm

for this error:

Error in ./src/App.js
Syntax error: /Users/goshakkk/Projects/oss/create-react-app/demo/src/App.js: Unexpected token (11:38)
   9 |         <div className="App-header">
  10 |           <img src={logo} className="App-logo" alt="logo" />
> 11 |           <h2>Welcome to React z</h2><<div
     |                                       ^
  12 |         </div>
  13 |         <p className="App-intro">
  14 |           To get started, edit <code>src/App.js</code> and save to reload.
 @ ./src/index.js 11:11-27
  • Possible to supply a custom image to webpack-notifier, could be React logo. Considerations: eject will have to copy the logo as well. Might be okay though.

  • Custom title is supported by webpack-notifier. "React" might be a bit too broad, "React Scripts" or "Create React App" or "React CLI" might be better.

  • Custom error formatter is not supported, plus terminal coloring artifacts are present ([4m). Not currently possible to supply a custom formatter to webpack-notifier. Ideally the message should have been

    Error: ./App.js
    Unexpected token (11:38)

    Full path & parts of stack trace seem irrelevant for the notification. Error message is much more informative.

Uses webpack-notifier under the hood. No setup necessary.
Supports Notification Center on OS X, Toaster on Windows 8 and later.
Falls back to Growl if present.

Ref facebook#89.
@ghost
Copy link

ghost commented Jul 23, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost
Copy link

ghost commented Jul 23, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jul 23, 2016
@lacker
Copy link
Contributor

lacker commented Jul 24, 2016

Why do you want a system notification for every error? - they are already displayed from the npm start process. To me this seems like a customization that some but not most developers will want, and so the right thing to do if you would like system notifications for each error is to eject and then customize webpack accordingly, rather than turn it on for everyone by default.

@gaearon
Copy link
Contributor

gaearon commented Jul 24, 2016

I’m not sure we’ll want to go with this but I’d like to try it out this week and see what it feels like.
If notifications can be muted on system level, maybe we could include this by default.
But I haven’t tried it so can’t say whether it’s annoying or useful to me.

@vjeux
Copy link
Contributor

vjeux commented Jul 24, 2016

By default notifications are turned off from 8pm to 8am on mac, which is very confusing when it stops working for no apparent reason :(

Unrelated but this little script is awesome: http://frantic.im/notify-on-completion

@gaearon
Copy link
Contributor

gaearon commented Aug 2, 2016

Thanks for the PR. I think we won’t go ahead with this as it seems a little too intrusive.

@gaearon gaearon closed this Aug 2, 2016
@lock lock bot locked and limited conversation to collaborators Jan 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants