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

[Improvement]: update our docs to use recommended alternatives to deprecated cargo watch #1944

Closed
1 task done
oddgrd opened this issue Dec 30, 2024 · 15 comments · Fixed by shuttle-hq/shuttle-docs#318
Closed
1 task done
Assignees
Labels
S-Accepted This will be worked on T-Improvement Improvement or addition to existing features

Comments

@oddgrd
Copy link
Contributor

oddgrd commented Dec 30, 2024

Describe the improvement

Cargo watch will eventually start failing due to no longer being maintained, so it would be good to update our docs to the recommended alternatives. I do like the one-liner we had, but it still requires the user to install a separate binary, so it still require some effort either way. We should consider having both in the docs, the option to use watchexec and the one-liner you can use it with, as well as a suggestion to use bacon, with a proposed config to make it convenient to use with shuttle.

Duplicate declaration

  • I have searched the issues and this improvement has not been requested before.
@oddgrd oddgrd added T-Improvement Improvement or addition to existing features S-Accepted This will be worked on labels Dec 30, 2024
@c-git
Copy link
Contributor

c-git commented Dec 30, 2024

Thanks, got it. Addling the link to the file to be updated https://github.com/shuttle-hq/shuttle-docs/blob/main/docs/local-run.mdx#development-tips

@c-git
Copy link
Contributor

c-git commented Jan 2, 2025

bacon may actually land one liners do we want to do the update after we know how that goes (PS. I don't know how long it would take to land)?

I'm just wondering if it's worth the wait?

@oddgrd
Copy link
Contributor Author

oddgrd commented Jan 3, 2025

It's no rush, cargo watch will keep working for a while, so feel free to wait on that if you'd prefer that! It looks quite close too!

@c-git
Copy link
Contributor

c-git commented Jan 3, 2025

Ok thanks, he's going to release a experimental version. I'll write up the docs once that's out and we'll just wait to merge after it stabilizes. Writing it up will help test the design to see how it feels. Once the draft is ready I'll reach out for feedback to see how you find it looks.

@oddgrd
Copy link
Contributor Author

oddgrd commented Jan 3, 2025

Sounds great, thank you!

@jonaro00
Copy link
Member

@c-git I saw Canop/bacon#296 got merged and released. Does that mean we can now add a shuttle watch that executes a bacon oneliner with shuttle run inside, without needing a config file? Or are we looking for some other solution here?

@c-git
Copy link
Contributor

c-git commented Jan 18, 2025

Yep it works now. I'll try to write it up either over the weekend or next week God willing. It's not really one line but it's one command.

@c-git
Copy link
Contributor

c-git commented Jan 18, 2025

It will look something like this https://dystroy.org/bacon/cookbook/#bacon-cli-snippets

bacon -j cli-test --config-toml '
[jobs.cli-test]
command = [
  "sh",
  "-c",
  "echo \"hello $(date +%H-%M-%S)\"; cargo run",
]
need_stdout = true
allow_warnings = true
background = false
on_change_strategy = "kill_then_restart"
kill = ["pkill", "-TERM", "-P"]'

@c-git
Copy link
Contributor

c-git commented Jan 18, 2025

@c-git I saw Canop/bacon#296 got merged and released. Does that mean we can now add a shuttle watch that executes a bacon oneliner with shuttle run inside, without needing a config file? Or are we looking for some other solution here?

I'm sorry I didn't read your question carefully. I thought you were talking about updating the instructions. I reread it and see that you want to wrap a bacon command. Yes that's also possible but bacon would have to be installed or included as a library inside of the shuttle CLI. I'm partial to it being separate instead of used as a library to allow users more flexibility but I see advantages to both approaches.

I'll include the bacon command I use to run shuttle the next time I go on the computer.

@c-git
Copy link
Contributor

c-git commented Jan 18, 2025

This is the command that would run shuttle with proper stop and restart

bacon -j shuttle --config-toml '
[jobs.shuttle]
command = ["shuttle", "run"]
need_stdout = true
allow_warnings = true
background = false
on_change_strategy = "kill_then_restart"
kill = ["pkill", "-TERM", "-P"]'

Let me know how you want to proceed. Updating the docs, change to the CLI or both?

@jonaro00
Copy link
Member

I think a shuttle watch or shuttle run --watch that calls this command on bacon if it exists would be nice. Including bacon as a lib would be too bloated IMO. We can also have the command written out in the related docs for anyone who wants to experiment.
WDYT @oddgrd ?

@oddgrd
Copy link
Contributor Author

oddgrd commented Jan 21, 2025

I think a shuttle watch or shuttle run --watch that calls this command on bacon if it exists would be nice. Including bacon as a lib would be too bloated IMO. We can also have the command written out in the related docs for anyone who wants to experiment. WDYT @oddgrd ?

I would say updating the docs to reflect how to do local dev runs with Bacon directly is a great start, both the simplest way to do it and how to configure it further if needed.

The idea of having a Shuttle CLI command that calls the bacon binary, if installed, is also worth considering. It might not be a great experience for a user to be told to install another binary after they try to run one of our CLI commands, but then again, it would also make the possibility of local dev runs more discoverable. Either they have bacon installed and it just works, or they are directed to the docs for how to get it working.

TLDR: I think updated docs would be great, I'm a bit torn on the shuttle watch wrapping bacon command, but open to it if you guys think we should proceed.

@jonaro00
Copy link
Member

We could highlight that it's an optional add-on (and an "unofficial" watch functionality) by calling it shuttle run --bacon (doc: "Run bacon for automatic reloads (separate install required)"). If bacon is not installed, it can tell you what needs to be done (docs link or direct instructions).

@jonaro00
Copy link
Member

jonaro00 commented Jan 21, 2025

@c-git We can start by adding a "cargo-watch is no longer maintained, but still works" to the current section on the docs, then add a "Live reload backend with bacon" section above it, with this snippet.
The discussion whether to add it to the CLI is moved to #1967, and we'll complete this one.

@c-git
Copy link
Contributor

c-git commented Jan 21, 2025

Ok thank you. Will make the update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Accepted This will be worked on T-Improvement Improvement or addition to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants