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

[feat] implement notify proxy to reload browser #661

Merged
merged 13 commits into from
Apr 4, 2024
Merged

Conversation

jackielii
Copy link
Contributor

fix #656

@jackielii
Copy link
Contributor Author

Let me know if anything is needed. I plan add some example setups for reloading script in typical set. E.g. tailwindcss + npm build + templ + go live reload example. But I think that's better suited in the docs using another PR

@a-h
Copy link
Owner

a-h commented Apr 2, 2024

It looks great! Thanks a lot for this.

I'd love to see an integration test where it starts a web server to listen to the POST request, runs the CLI go run cmd/templ --notify-proxy, and validates that the post was received.

If you haven't got time, I can do that though, just let me know!

@jackielii
Copy link
Contributor Author

It looks great! Thanks a lot for this.

I'd love to see an integration test where it starts a web server to listen to the POST request, runs the CLI go run cmd/templ --notify-proxy, and validates that the post was received.

If you haven't got time, I can do that though, just let me know!

I can write a http recorder test to test the new route handle with the notify function call. Would that be enough? Probably much easier to write?

@a-h
Copy link
Owner

a-h commented Apr 2, 2024

Great idea. The main reason is to protect against future regressions.

@jackielii
Copy link
Contributor Author

added a httptest.Server test. Hopefully I didn't miss anything. I find it wired that URL appears in the handler: https://github.com/a-h/templ/blob/main/cmd/templ/generatecmd/proxy/proxy.go#L29. I know it's being used inside the proxy.Handler attached methods, but handler doesn't really care where the request come from right?

cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
cmd/templ/generatecmd/proxy/proxy_test.go Outdated Show resolved Hide resolved
@a-h a-h merged commit 022a8b0 into a-h:main Apr 4, 2024
4 checks passed
@jackielii jackielii mentioned this pull request Apr 4, 2024
Comment on lines +85 to +98
### Triggering hot reload from outside `templ generate --watch`

If you want to trigger a hot reload from outside `templ generate --watch` (e.g. if you're using `air`, `wgo` or another tool to build, but you want to use the templ hot reload proxy), you can use the `--notify-proxy` argument.

```shell
templ generate --notify-proxy
```

This will default to the default templ proxy address of `localhost:7331`, but can be changed with the `--proxybind` and `--proxyport` arguments.

```shell
templ generate --notify-proxy --proxybind="localhost" --proxyport="8080"
```

Copy link

@leandergangso leandergangso Apr 6, 2024

Choose a reason for hiding this comment

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

Great work, this is a nice addition to the templ CLI, will be using this once it's released.
Maybe even replace --watch?

I was thinking that a change to the explanation, to make it more clear, could help this feature be more utilized when it's released.

By specifying that --notify-proxy takes priority over other flags, and will be exec just once, can help developers better understand the usage for this cmd.
Also, the first time I read this text, it felt like I should add --notify-proxy as a flag to the templ generate --watch cmd, which would end in an unexpected outcome. (perhaps just my thinking tho :P )

Could also mention that --notify-proxy only works if --proxy is used. You kinda already does this and it's somewhat self explanatory, but one extra sentence could clear up any misunderstandings?

PS: some of this can maybe also be applied to the --notify-proxy CLI help text.

just some thoughts on my part.

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.

cmd: add --notify to generate --watch in cli
3 participants