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 a property for configuring additional livereload paths #41566

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

itsAkshayDubey
Copy link
Contributor

This PR enhances the functionality related to issue #40342 by introducing configurable additional paths for live reload. This feature mirrors the existing capability for configuring additional paths for application restart.

@mhalbritter mhalbritter changed the title Add a property for configuring additional livereload paths #40342 Add a property for configuring additional livereload paths Jul 29, 2024
@mhalbritter mhalbritter self-assigned this Jul 29, 2024
@mhalbritter
Copy link
Contributor

Hey, thanks for the PR. I've looked at the changes and have some feedback:

You're creating a new FileSystemWatcher bean to watch for live reload changes. However, the restart functionality already has a FileSystemWatcher to watch for changes. It would be good to reuse that FileSystemWatcher so that it can trigger a restart (if restart paths change) or trigger a live reload (if reload paths change).

That would mean that pollInterval and quietPeriod can't be configured separately, but I don't think this is a big deal.

Do you have the time to implement this idea?

@mhalbritter mhalbritter added the status: waiting-for-feedback We need additional information before we can continue label Jul 30, 2024
@itsAkshayDubey
Copy link
Contributor Author

Hi @mhalbritter , sure I'll submit my PR super soon.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Aug 2, 2024
@mhalbritter mhalbritter added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Aug 5, 2024
@philwebb
Copy link
Member

Hi @itsAkshayDubey, any luck implementing the requested changes?

@itsAkshayDubey
Copy link
Contributor Author

On it, will submit my PR in next 2 days.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 3, 2024
@itsAkshayDubey
Copy link
Contributor Author

Hi @mhalbritter @philwebb , kindly have a look.

@mhalbritter
Copy link
Contributor

Hello @itsAkshayDubey. Thanks for taking another go at it, but unfortunately, the changes aren't what we are looking for. Now we have two new FileSystemWatcher. In that comment above I suggested that we instead use the already existing FileSystemWatcher bean which is already there to watch for restart changes and use it to also watch for livereload changes. That way we would only have one FileSystemWatcher.

Do you want to give it another go or should we abort here?

@mhalbritter mhalbritter added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 9, 2024
@itsAkshayDubey
Copy link
Contributor Author

Hi @mhalbritter,
Thank you for your input. I'll definitely address this. However, I would appreciate a bit of clarification to proceed effectively.
As I understand it, RestartConfiguration is a conditional bean based on the spring.devtools.restart.enabled property. To access the FileSystemWatcher from RestartConfiguration, we would need the bean to be available. In the current implementation, I am utilising FileSystemWatcher when the RestartConfiguration bean is present, or otherwise, creating a new instance.
Could you kindly advise on the best approach to consistently use the same FileSystemWatcher from the RestartConfiguration bean, regardless of its conditional nature?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 9, 2024
@mhalbritter
Copy link
Contributor

mhalbritter commented Sep 9, 2024

The FileSystemWatcher is created in org.springframework.boot.devtools.autoconfigure.LocalDevToolsAutoConfiguration.RestartConfiguration#newFileSystemWatcher. Right now, it uses org.springframework.boot.devtools.autoconfigure.DevToolsProperties.Restart#additionalPaths to configure additional paths.

My idea was that we add the org.springframework.boot.devtools.autoconfigure.DevToolsProperties.Livereload#additionalPaths to the same FileSystemWatcher, and then check in in the listeners if the change should lead to a restart or a livereload. Then we only need one FileSystemWatcher.

I haven't looked into detail how we need to move the auto-configuration classes and conditions around, but I guess at least for the FileSystemWatcher we need a condition which is true if either spring.devtools.restart.enabled=true OR spring.devtools.livereload.enabled=true. This can be done with a org.springframework.boot.autoconfigure.condition.AnyNestedCondition.

I'm not 100% sure that this will work, because there's a lot going on in the restart functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants