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

add autoreload to trunk serve #191

Merged
merged 2 commits into from
Aug 12, 2021
Merged

add autoreload to trunk serve #191

merged 2 commits into from
Aug 12, 2021

Conversation

kcking
Copy link
Contributor

@kcking kcking commented Jul 2, 2021

Thanks for building trunk! It's such an awesome tool.

I took a pass at implementing a simple autoreload that is triggered by an injected websocket script. It is exposed by a --autoreload flag that defaults to false. It looks like my rustfmt did some collateral formatting in a couple of files, I can manually revert it if need be.

Let me know what you think!

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will :).

Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

@kcking thanks for the PR! Stoked to see this feature being implemented. A few initial comments, let me know what you think.

Cargo.toml Outdated Show resolved Hide resolved
src/autoreload.js Outdated Show resolved Hide resolved
src/config/models.rs Outdated Show resolved Hide resolved
src/watch.rs Outdated Show resolved Hide resolved
src/serve.rs Outdated Show resolved Hide resolved
src/serve.rs Outdated Show resolved Hide resolved
src/serve.rs Outdated Show resolved Hide resolved
@kcking
Copy link
Contributor Author

kcking commented Jul 28, 2021

Thanks for the review @thedodd! All your comments sgtm, I'll address them shortly.

@kcking
Copy link
Contributor Author

kcking commented Jul 28, 2021

Ok just addressed those comments, let me know if there is anything else!

I changed the flag name from autoreload to no-autoreload since it is now enabled by default. This seemed simpler than using --autoreload=false to disable it, but let me know if you think there's a better way.

@thedodd thedodd mentioned this pull request Jul 30, 2021
6 tasks
@thedodd
Copy link
Member

thedodd commented Aug 6, 2021

@kcking sorry to make your PR more difficult! The Tokio cut over is complete, so there are a few conflicts and such. I do think the code will be a bit more simple now though.

On a high level, given Tokio's significantly better messaging support, we should probably use a tokio::sync::watch channel, which new WebSocket connections will simply subscribe to and then listen for changes on the channel. For this initial autoreload support, I don't think we need to propagate compiler error messages or anything like that. We can just emit a new value over the channel upon a successful build.

In the future, we can actually propagate a structure message over the websockets so that we can create a diagnostics overlay or drop the info in the console and such.

@kcking
Copy link
Contributor Author

kcking commented Aug 6, 2021 via email

triggered by an injected websocket script
disabled with --no-autoreload (default false)
@kcking
Copy link
Contributor Author

kcking commented Aug 8, 2021

@thedodd alrighty just updated this PR to support axum/tokio. Let me know if there's any other changes on your mind :)

thedodd
thedodd previously approved these changes Aug 9, 2021
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Awesome! I'm going to merge this in a few hours, I need to go AFK for a bit though. Really excellent work @kcking! Thanks!

Notes for myself for after I merge this:

  • Add docs about this feature to the site.
  • Add docs for this config var to the default Trunk.toml.
  • Add to config cascade.
  • Add a todo/note in the code about future emission of error info over the socket.
  • Probably move code injector the HTML pipeline.

Cargo.toml Outdated Show resolved Hide resolved
@kcking
Copy link
Contributor Author

kcking commented Aug 10, 2021

Awesome! I'm going to merge this in a few hours, I need to go AFK for a bit though. Really excellent work @kcking! Thanks!

Woo! Thank you for the review and for the awesome tool :)

@thedodd thedodd merged commit fa58309 into trunk-rs:master Aug 12, 2021
@thedodd
Copy link
Member

thedodd commented Aug 12, 2021

🎉

@thedodd
Copy link
Member

thedodd commented Aug 12, 2021

BTW, @kcking you should totally join our discord server so that I can give you a shoutout for this work :)!

@chetankhilosiya
Copy link

Great work. I am eagerly waiting for this feature to become available as release.

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

Successfully merging this pull request may close these issues.

3 participants