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

Improve watch mode to reduce recompilation #366

Merged
merged 12 commits into from
Jan 7, 2024
Merged

Conversation

stephenafamo
Copy link
Contributor

Implementation for #318 and #340

@stephenafamo
Copy link
Contributor Author

Not 100% ready. Needs some cleaning up and testing.

Also, I can't figure out why, but additional builds in watch mode seem to ignore the string extraction.

@stephenafamo
Copy link
Contributor Author

additional builds in watch mode seem to ignore the string extraction.

It seems some "phantom templ generate" is also running in the background and it overwrites the file.
I am not sure what invokes it, but it seems to use some existing templ because it ignores things like adding a custom comment at the end of each line.

@stephenafamo
Copy link
Contributor Author

but it seems to use some existing templ

This was the issue. I had a running templ generate --watch that wasn't killed properly. The PR works great 👌🏾

@stephenafamo
Copy link
Contributor Author

The PR is now ready to be reviewed @a-h

Side note: I also modified writeText to use WriteLiteralString

@stephenafamo
Copy link
Contributor Author

Waiting on a review 🙏🏾 @a-h

@a-h
Copy link
Owner

a-h commented Jan 4, 2024

Apologies. I want to have a good sit down and really understand it. With the holidays, I've just been picking up small stuff.

I will get on it though, I'm excited about the change. Think it will be a big improvement to dev performance.

Copy link
Owner

@a-h a-h left a comment

Choose a reason for hiding this comment

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

Wow, really awesome work, thanks so much for putting this together, and I'm really sorry it took me so long to get to looking at this.

I think there's a few bits to clear up, but it's brilliant.

I've asked for a few changes, and put in a few suggestions. Once that's sorted, I'll merge it, and put together an integration test as per #381 (comment)

Again, thanks so much.

cmd/templ/generatecmd/main.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/main.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/main.go Show resolved Hide resolved
cmd/templ/generatecmd/main.go Show resolved Hide resolved
cmd/templ/generatecmd/main.go Outdated Show resolved Hide resolved
benchmarks/templ/template_templ.go Outdated Show resolved Hide resolved
runtime.go Outdated Show resolved Hide resolved
generator/rangewriter.go Outdated Show resolved Hide resolved
generator/rangewriter.go Outdated Show resolved Hide resolved
generator/rangewriter.go Outdated Show resolved Hide resolved
@stephenafamo
Copy link
Contributor Author

Ready for another review @a-h

Copy link
Owner

@a-h a-h left a comment

Choose a reason for hiding this comment

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

The mutex lock/unlock needs to move to the getWatchedStrings function to properly protect against reading and writing the map concurrently since there's a chance of a "concurrent map read and write" error, but other than that, it's awesome.

I'll add the mutex change in a subsequent commit.

@a-h a-h merged commit eac4954 into a-h:main Jan 7, 2024
4 checks passed
@a-h
Copy link
Owner

a-h commented Jan 7, 2024

Brilliant work, thanks a lot. I think it's worth a blog post to explain the improvement, and how it's a potential game changer for development experience with templ. If you want to write it, go ahead, if not, I will - just let me know.

The next thing to deal with is to switch out the very basic "walk the directory every second" with a filesystem watcher implementation. I couldn't get fsnotify to work properly on my Mac, but maybe I was using it wrong.

With that in place, the reload time will be incredible...

@philippefuentes
Copy link

Brilliant work, thanks a lot. I think it's worth a blog post to explain the improvement, and how it's a potential game changer for development experience with templ. If you want to write it, go ahead, if not, I will - just let me know.

The next thing to deal with is to switch out the very basic "walk the directory every second" with a filesystem watcher implementation. I couldn't get fsnotify to work properly on my Mac, but maybe I was using it wrong.

With that in place, the reload time will be incredible...

Hi @a-h what was the problems you encountered with fsnotify ?

@stephenafamo
Copy link
Contributor Author

Brilliant work, thanks a lot. I think it's worth a blog post to explain the improvement, and how it's a potential game changer for development experience with templ. If you want to write it, go ahead, if not, I will - just let me know.

I will write something about this. It's been too long since I wrote a blog post and this definitely deserves one.

@stephenafamo stephenafamo deleted the hot-watch branch January 17, 2024 05:44
@a-h
Copy link
Owner

a-h commented Jan 17, 2024

@philippefuentes the issue I had was inconsistent notifications. Sometimes it would work, sometimes not.

@a-h
Copy link
Owner

a-h commented Jan 17, 2024

Basically, the watch implementation needs some automated integration tests, including one that can be ran for extended periods to identify issues that crop up over time.

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