[assetserver] Introduce middleware and extract options #2016
Conversation
stffabi
force-pushed
the
feature/assetserver-middleware
branch
2 times, most recently
from
5cb06bf
to
da9a909
Compare
stffabi
force-pushed
the
feature/assetserver-middleware
branch
2 times, most recently
from
1e8e613
to
ff508aa
Compare
|
I'm keen to know how this plays into the "server/hybrid target" that @tmclane is aiming for. |
| return opts | ||
| } | ||
|
|
||
| return &assetserver.Options{ |
There was a problem hiding this comment.
Considering this is a small struct and it's not getting called a whole heap of times, I'm wondering if it's worth changing the signatures to accept a struct rather than a struct pointer? At this point in the code, it's gone from being optional to a mandatory struct. Just a thought as it's easier to reason about knowing there won't be nil pointer dereferencing. Thoughts?
There was a problem hiding this comment.
I think we might change the return value to a struct type instead instead of a struct pointer.
For the input I'm not sure if we can do that, see my reply to your comment on the options struct.
There was a problem hiding this comment.
I have changed the return value to a struct.
| // Deprecated: Use AssetServer.Handler instead. | ||
| AssetsHandler http.Handler | ||
| // AssetServer configures the Assets for the application | ||
| AssetServer *assetserver.Options |
There was a problem hiding this comment.
If we made this a non-pointer, wouldn't the fields be testable for nil anyway? Just wondering if the zero value for this might be a better option.
There was a problem hiding this comment.
I think that would make it impossible to distinct some cases, e.g. if:
Assets: embedFS,
AssetServer: assetserver.Options{}This would mean AssetServer.Assets is nil. Does this now mean the developer wanted to have nil assets but just forgot removing it from the deprecated Assets or did they want the Assets and forgot to add them to the AssetServer struct.
For the check of inconsistency it's much easier to have struct-pointer. If it's nil one can be assured the developer didn't want to change anything and was using the deprecated options. If it has been set, the developer wanted to migrate to the AssetServer option, but forgot to remove the old assignments.
There was a problem hiding this comment.
But yeah, if migration is over I would also opt for having a struct instead of a struct pointer there.
There was a problem hiding this comment.
A couple of small questions but looking good. I think we should get @mholt to sign off on it 😄
stffabi
force-pushed
the
feature/assetserver-middleware
branch
from
ff508aa
to
f2078df
Compare
|
Aye, this is working well for me as far as I can tell! Thank you thank you thank you for this, it allows me to use vanilla JS for my frontend which vastly simplifies things, and I can render "static" HTML pages dynamically using Go templates (which I also use to include HTML components onto my HTML pages). I'll let you know if I run into any hiccups further on. |
| type Middleware func(next http.Handler) http.Handler | ||
|
|
||
| // ChainMiddleware allows chaining multiple middlewares to one middleware. | ||
| func ChainMiddleware(middleware ...Middleware) Middleware { |
There was a problem hiding this comment.
FYI, I have not found myself using this, but I can't say it wouldn't be useful to others (or even myself in the future). Just wanted to let you know that I was able to do what I needed with a single middleware. Since I control all of the middleware handlers, I can just multiplex them from my single middleware function.
There was a problem hiding this comment.
Thanks for letting me know. I didn't want to have an array of middleware in the API, so I thought this would round it up with this for people that want to chain multiple middlewares.
stffabi
force-pushed
the
feature/assetserver-middleware
branch
from
f2078df
to
674f3e9
Compare
|
Just updated the PR with docs and the template updates. @leaanthony would you mind taking a look into those commits? 🙏 |
stffabi
force-pushed
the
feature/assetserver-middleware
branch
from
674f3e9
to
a2938fa
Compare
|
@mholt did you see any problems with this PR, which would need a fix? |
|
Nothing sticks out to me as problematic. So far I am using it successfully! Thanks so much for working on this. Super appreciated. |
…handler and middleware In prod mode we can't support WebSockets so make sure the assets handler and middleware never see WebSockets in dev mode.
stffabi
force-pushed
the
feature/assetserver-middleware
branch
from
a2938fa
to
909a542
Compare
|
Awesome, you're welcome. So I'm going to merge this PR. |
|
This is fabulous, thank you. Has been working great. |
This PR introduces a HTTP middleware for the AssetServer, furthermore it changes the API for defining the AssetServer config.
Fixes #2007