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

Add nextjournal/beholder as an alternative watcher #268

Merged
merged 7 commits into from
Feb 1, 2022
Merged

Add nextjournal/beholder as an alternative watcher #268

merged 7 commits into from
Feb 1, 2022

Conversation

oxalorg
Copy link
Member

@oxalorg oxalorg commented Jan 26, 2022 •
edited
Loading

hawk is using an old version of JNA which breaks file watching on m1 macs.
The alternative was to use :polling or :java but they were either too slow
or consumed lots of battery

beholder seems to be maintained, fast, and a much cleaner approach.

figwheel has also shifted to using beholder instead of hawk. shadow-cljs
also got rid of hawk

maybe beholder should just replace hawk altogether?

  • add beholder as default FS
  • hawk can be used using a configuration param :kaocha.watch/type :hawk but show DEPRECATED warnings
  • update docs and changelog

cc: #151 , thheller/shadow-cljs@f3b89b5 , bhauman/figwheel-main#299

@oxalorg
add nextjournal/beholder as an alternative watcher
6fb11ad 
hawk is using an old version of JNA which breaks file watching on m1 macs.
The alternative was to use :polling or :java but they were either too slow
or consumed lots of battery

beholder seems to be maintained, fast, and a much cleaner approach.

figwheel has also shifted to using beholder instead of hawk. shadow-cljs
also got rid of hawk

maybe beholder should just replace hawk altogether?
@codecov
Copy link

codecov bot commented Jan 26, 2022 •
edited
Loading

Codecov Report

Merging #268 (9ecc0ec) into main (005e91f) will decrease coverage by 0.27%.
The diff coverage is 74.07%.

❗ Current head 9ecc0ec differs from pull request most recent head de158bd. Consider uploading reports for the commit de158bd to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #268      +/-   ##
==========================================
- Coverage   75.62%   75.35%   -0.28%     
==========================================
  Files          51       51              
  Lines        2659     2678      +19     
  Branches      246      250       +4     
==========================================
+ Hits         2011     2018       +7     
- Misses        495      503       +8     
- Partials      153      157       +4
Flag Coverage Δ
integration 57.28% <11.11%> (-0.34%) ⬇️
unit 69.11% <74.07%> (-0.24%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/kaocha/watch.clj 78.67% <74.07%> (-3.10%) ⬇️
src/kaocha/core_ext.clj 64.28% <0.00%> (-4.77%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 005e91f...de158bd. Read the comment docs.

@plexus
Copy link
Member

plexus commented Jan 27, 2022 via email

@alysbrooks alysbrooks mentioned this pull request Jan 27, 2022
Closed
@alysbrooks
Copy link
Member

I guess the only thing we'd lose is the ability to fall back to polling, but hopefully no one is relying on that.

@alysbrooks
Copy link
Member

I realize this is a draft, but I was already looking at Beholder's source and noticed something. Does Beholder have any options? Maybe we can delete that configuration option?

@oxalorg
defaults to beholder, works on all platforms natively
fe29847 
we can remove hawk completely in a few versions, until then folks
can manually configure `:kaocha.watch/type :hawk` to use hawk

deprecation notice is warned when either hawk is configured
or if hawk-opts are found in the config
@oxalorg
update docs and changelog
b75e0f4
@oxalorg oxalorg marked this pull request as ready for review January 31, 2022 12:09
@oxalorg
remove ::beholder-opts
fa928c8 
Right now beholder has no configuration options
@oxalorg
remove opts from beholder call
2922957
alysbrooks
alysbrooks approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@alysbrooks alysbrooks left a comment

Choose a reason for hiding this comment

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

Looks good! I have some small changes you can take or leave but nothing too major.

src/kaocha/watch.clj Show resolved Hide resolved
src/kaocha/watch.clj Show resolved Hide resolved
doc/07_watch_mode.md Show resolved Hide resolved
doc/07_watch_mode.md Outdated Show resolved Hide resolved
doc/07_watch_mode.md Outdated Show resolved Hide resolved
oxalorg and others added 2 commits February 1, 2022 00:22
@oxalorg @alysbrooks
Update doc/07_watch_mode.md
e5c6583 
Co-authored-by: A Brooks <alys@gaiwan.co>
@oxalorg @alysbrooks
Update doc/07_watch_mode.md
de158bd 
Co-authored-by: A Brooks <alys@gaiwan.co>
@oxalorg
Copy link
Member Author

oxalorg commented Jan 31, 2022

@alysbrooks Thanks for the suggestions, made those changes. Feel free to merge it in :)

@alysbrooks alysbrooks merged commit a55bc3f into main Feb 1, 2022
@oxalorg oxalorg mentioned this pull request Feb 4, 2022
Closed
This was referenced Mar 9, 2022
Merged
Merged
@jeffp42ker jeffp42ker mentioned this pull request Aug 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@plexus plexus plexus left review comments

@alysbrooks alysbrooks alysbrooks approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@oxalorg @plexus @alysbrooks