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.

Sign up for GitHub

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

Jump to bottom

WIP refactor(saber): add node-notifier #514

Closed
wants to merge 8 commits into from
Closed

WIP refactor(saber): add node-notifier #514

wants to merge 8 commits into from
+35 −5

Conversation

tyankatsu0105
Copy link

@tyankatsu0105 tyankatsu0105 commented Oct 9, 2019 •
edited
Loading

  • Add node-notifier with yarn workspace
  • Write test Not required
  • Add icon for packages/assets
  • Refactor exist code

Summary

close #509

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Docs
  • Build-related changes
  • Other, please describe:

If changing the UI/CSS related code, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

@netlify
Copy link

netlify bot commented Oct 9, 2019 •
edited
Loading

Deploy preview for saber ready!

Built with commit 2dad224

https://deploy-preview-514--saber.netlify.com

@tyankatsu0105
Copy link
Author

@egoist
I don't know how to test :_(

@nblthree
Copy link
Member

nblthree commented Oct 9, 2019

Just add a comment here https://github.com/saberland/saber/blob/master/website/saber-node.js

egoist
egoist reviewed Oct 9, 2019
View reviewed changes
packages/saber/lib/plugins/extend-node-api.js Outdated
@@ -86,7 +88,7 @@ exports.apply = api => {
)
// Because you might also update webpack config in saber-node.js
// Which we can't (?) automatically reload
log.warn(`You probably need to restart the server.`)
notifier.notify('You probably need to restart the server.')
Copy link
Collaborator

@egoist egoist Oct 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

we should also output warning in console

Copy link
Author

@tyankatsu0105 tyankatsu0105 Oct 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oops. Sorry.

Copy link
Member

Choose a reason for hiding this comment

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

We should set Saber as the title

@tyankatsu0105
Copy link
Author

@MarchWorks
Thanks for the reply.
But I don't understand what you say...
Why do I add some comments that file?

@nblthree
Copy link
Member

nblthree commented Oct 10, 2019 •
edited
Loading

@tyankatsu0105 sorry, I got it wrong. There is no need for any test.

@egoist
Copy link
Collaborator

egoist commented Oct 10, 2019

We can also display the Saber logo in the notifier https://github.com/saberland/art/blob/master/icon/icon-alt.png

@egoist
Copy link
Collaborator

egoist commented Oct 10, 2019

The notifier should also be used for saber-config.js https://github.com/saberland/saber/blob/master/packages/saber/lib/plugins/watch-config.js

@tyankatsu0105
Copy link
Author

tyankatsu0105 commented Oct 10, 2019 •
edited
Loading

OK.
Code is probably this.

notifier.notify({
  title: 'Saber',
  icon: path.join(__dirname, 'saber.jpg'),
  message: 'You probably need to restart the server.'
})

extend-node-api.js can't import icon from saberland/art.
Can I put icon for plugins directory or something?

@egoist
Copy link
Collaborator

egoist commented Oct 10, 2019 •
edited
Loading

extend-node-api.js can't import icon from saberland/art.
Can I put icon for plugins directory or something?

You can put it inside saber/lib/assets directory

@egoist
Copy link
Collaborator

egoist commented Oct 10, 2019

We should make the message more detailed, like saber-node.js was changed, you need to restart the server

@tyankatsu0105
Copy link
Author

OK!
Thanks, @egoist .
I'll try doing that.

@tyankatsu0105 tyankatsu0105 changed the title WIP refactor(saber): add node-notifier refactor(saber): add node-notifier Oct 11, 2019
@tyankatsu0105
Copy link
Author

@egoist
I'm finished!
Would you mind reviewing this PR?

@krmax44
Copy link
Contributor

krmax44 commented Oct 12, 2019

What about including a button in the notification that allows restarting directly?

@tyankatsu0105
Copy link
Author

button in the notification that allows restarting directly

That’s interesting.
I'll try doing that.

@egoist
Copy link
Collaborator

egoist commented Oct 13, 2019 •
edited
Loading

  1. Just checked the install size of node-notifier: 5.5 MB, now I'm not sure if it is worth adding just for two warnings 🤔

  2. Not quite happy about the result of node-notifier, can we get rid of that default icon?

image

  1. Showing a notifier on macOS Catalina will request user permission, not sure if it's a good DX, what about showing the warning elsewhere? Like in the browser 🤔

@egoist
Copy link
Collaborator

egoist commented Oct 13, 2019 •
edited
Loading

@krmax44: What about including a button in the notification that allows restarting directly?

That would require Saber to be run in a child process, Next.js used to do this and auto-restart when config file is changed but it seems they removed this feature.

@tyankatsu0105
Copy link
Author

Sorry, I don't know how to restart server :_(

@tyankatsu0105
Copy link
Author

tyankatsu0105 commented Oct 14, 2019 •
edited
Loading

Oh. I just checked this comment.
I think this feature is not needed for saber.

Like in the browser

Um, could you give me an example?

@tyankatsu0105
Copy link
Author

@egoist
Thank you for your several opinions.
If you think unnecessary this feature, please close this PR. And then, let's open a new issue.

@nblthree
Copy link
Member

nblthree commented Oct 14, 2019 •
edited
Loading

@tyankatsu0105 check this PR #503 as an example of sending a notification to the browser. The difference is that you will have to send your own data from the development server and get it with eventSource

@tyankatsu0105 tyankatsu0105 changed the title refactor(saber): add node-notifier WIP refactor(saber): add node-notifier Oct 14, 2019
@tyankatsu0105
Copy link
Author

I close this PR.
Because this PR's purpose is incorrect for the issue.
re #514 (comment)

Maybe node-notifier doesn't meet the needs.

@tyankatsu0105 tyankatsu0105 deleted the add-notification branch October 27, 2019 07:16
@tyankatsu0105
Copy link
Author

Sorry that I couldn't be your help. :_(

@tyankatsu0105 tyankatsu0105 mentioned this pull request Oct 27, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nblthree nblthree nblthree left review comments

@egoist egoist egoist left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Show a notifier when it's required to restart server
4 participants
@tyankatsu0105 @nblthree @egoist @krmax44