labstack
Support for Attachment
With this PR I would like to add support for Attachment without having to define a dedicated handler for it.
I am not sure if it makes sense to add this. It takes only 2 extra lines (~60 characters) to have same functionality with handler and handler allows you to add things like extra headers for caching or other headers for older browser quirks (Internet Explorer)
e.Attachment("/walle", "_fixture/images/walle.png", "wall-e.png")
vs
e.GET("/walle", func(c echo.Context) error {
return c.Attachment("_fixture/images/walle.png", "wall-e.png")
})
Hi @aldas, You are right, but we can make the same considerations for the File function (https://echo.labstack.com/guide/static-files/#using-echofile)
My idea was to have the same experience, avoiding a dedicated handler
Yes, File() exists and Attachment would be very similar. Main difference is that File is already part of API and part of contract library needs to maintain. I personally would be cautious when adding methods to APIs that are very simple shortcuts because in future versions this is another method signature to maintain.
From my personal perspective there is big difference if we talk about methods for HTTP methods (GET/PUT etc) and methods to handle files. Everything related to HTTP files comes eventually to setting HTTP headers to get browsers to work correctly and you find soon then these shortcut methods are too restricting for your requirements (ala you are fighting with MS-IE/Apple browsers to open correctly your calendar file or have it correct name etc).
Ok @aldas, I was considering just the base case in order to avoid the boilerplate handler. For the needs that I encountered, I just wanted to make the browser automatically download an already generated file, no more.
I understand your doubts, feel free to close this.
Lets not close this yet. I feel like I am the party pooper here. It would be definitively useful for some use-cases and would not be something directly negative of adding it. As these are "permanent" decisions we need to be careful not to commit to API contracts to easily.
Browser support has improved quite a lot and most funky ones like IE6 do not exist anymore, which helps testing quite a bit.
Still I'm not sure the simple wrapper without any options is worth it, as it would leave quite some details about expiry, caching, filetype (mime), etc undefined.
If we are unsure, I would suggest to not complicate the API. But other feedback welcome.
@vishr @pafuent or others: Any more comments in favor of adding this wrapper? Otherwise I guess we should not merge this but rather add an example in the docs.
@vishr @pafuent or others: Any more comments in favor of adding this wrapper? Otherwise I guess we should not merge this but rather add an example in the docs.
@ilteoood Thanks for this PR, but it seems interest is too low to add the additional API surface.