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

Switch to streaming/event source mechanism for live reload #458

Merged
merged 2 commits into from
Nov 29, 2021

Conversation

jaredcwhite
Copy link
Member

Resolves #416 (maybe?)

Well this certainly ended up being an inordinate amount of work! Getting the streaming mechanism enabled wasn't too bad, but then I ran into a bad situation where Ctrl-C would hang until Puma determined the stream had completed (and the longer it would run, the longer it would hang!). It took quite a while to workaround that, and then I discovered long streams would also gunk up Puma's threads if enough pages were opened at once and you were clicking around, because of a delay between a page change and the socket (browser? Puma?) cleaning up and stopping the stream.

I think it's pretty balanced now, between the length of the stream before a browser-initiated reconnection and the interval checking the modification, as well as further modification checks in-browser between a previous stream and a reconnected stream. Good grief.

I tested in Safari and Firefox…haven't tried any other browsers yet.

@render
Copy link

render bot commented Nov 21, 2021

@render
Copy link

render bot commented Nov 21, 2021

@ayushn21
Copy link
Member

Damn, sounds like you went down quite the rabbit hole with this one 😬. I'm not sure there's a tangible benefit to this approach for the added complexity though 🤷. I know you mentioned that on Twitter as well, but honestly just for the effort put in, if it works, we should probably go for it and see what happens!!

@interrupted = false

class << self
attr_accessor :interrupted
Copy link
Member

Choose a reason for hiding this comment

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

Can we use ActiveSupport's mattr_accessor here by any chance?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question…I think mattr_accessor is particularly useful when object instances which have included a module want to access that module's accessor easily. No need for that here, so probably wouldn't need the overhead

@jaredcwhite
Copy link
Member Author

Having done some work recently on a project without this PR and seeing live_reload requests cluttering up the Network tab, I think this PR will be a good step in the right direction.

@jaredcwhite jaredcwhite merged commit 64f4f2f into main Nov 29, 2021
@jaredcwhite jaredcwhite deleted the improved-live-reload-streaming branch November 29, 2021 17:17
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.

Investigate improving live reloading in development
2 participants