Skip to content
This repository has been archived by the owner on Jul 7, 2023. It is now read-only.

Make watchman-processor librar-ish #133

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

ventuno
Copy link

@ventuno ventuno commented Aug 26, 2018

Most npm packages that run as a binary have also the ability to be imported in a project and run as a library.
It would be great if watchman-processor worked the same way, so that developers could extend its functionality and create new UIs or integrate it with other tools if needed. See watchman-processor-tray-icon [1] as an example that uses watchman-processor as a library to create a bitbar plugin [2] and send notifications to the notification center (see also this better implementation of a tray icon using electron [3]).

Changes summary:

  1. Modify WatchmanProcessor.ts to emit node events instead of calling directly Terminal.ts;
  2. In bin/watchman-processor listen to these events and call the corresponding Terminal.ts methods;
  3. Update tests accordingly.

@markis, let me know if you like this idea, I think it would be a pretty powerful extension to this great package. More than happy to switch to more a appropriate architecture if needed, I kind of wanted to write down the code to convey the idea better.

[1] https://github.com/ventuno/watchman-processor-tray-icon/blob/f73d873463a3f682c649a1ca80b8b76cb2cbb11e/bitbar-plugin.js
[2] https://github.com/matryer/bitbar#writing-plugins
[3] ventuno/watchman-processor-tray-icon#1

@coveralls
Copy link

coveralls commented Aug 26, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 874258e on ventuno:make-watchman-processor-librar-ish into 98052a5 on markis:master.

@markis
Copy link
Owner

markis commented Aug 26, 2018

Wow, cool! I love the idea of integrating this with bitbar.

I am not a fan of magic strings and I would not want the internals of this to be dependent on them. It inevitably leads to this: f7d9326#diff-0ba683aead6f4617bf0950fcc8b0913dL72

I think all of this could have been done simply by building a different "terminal" class and just injecting it instead of rewiring all the internals to use events.

@ventuno
Copy link
Author

ventuno commented Aug 26, 2018

Thanks @markis how do you suggest I proceed? How would the alternative "terminal" class emit events to the outside world and how do developers inject it?
Eventing/hooks seemed the most standard approach.
I could expose event strings as constants and improved tests should protect from "magic" strings error.

@ventuno
Copy link
Author

ventuno commented Aug 29, 2018

@markis I spent some more time on this to use an enum instead of just strings to trigger/listen to events. I tried to improve tests, but due the overall asynchronicity of the whole program it's a little difficult to do so without introducing flakiness. Let me know what you think.

@ventuno
Copy link
Author

ventuno commented Sep 5, 2018

@markis ping?

@ventuno
Copy link
Author

ventuno commented Sep 5, 2018

Sure @markis, just wanted to know if this had still a chance :-). Good luck with your sale!

@ventuno
Copy link
Author

ventuno commented Dec 15, 2018

Hey @markis any chance you could get to this? Maybe in the new year?

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

Successfully merging this pull request may close these issues.

None yet

3 participants