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

PR: add url for favicon middleware, for correct handling different of… #2231

Merged
merged 8 commits into from
Feb 3, 2023
Merged

PR: add url for favicon middleware, for correct handling different of… #2231

merged 8 commits into from
Feb 3, 2023

Conversation

0xdeface
Copy link
Contributor

@0xdeface 0xdeface commented Nov 23, 2022

Description

Presently ico url might be only /favicon.ico but some resource wants url exactly /favicon.svg for svg format.
I added new property to favicon config and default value with /favicon.ico. Also i changed docs for this middleware

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist:

  • For new functionalities I follow the inspiration of the express js framework and built them similar in usage
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation - https://github.com/gofiber/docs for https://docs.gofiber.io/
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • If new dependencies exist, I have checked that they are really necessary and agreed with the maintainers/community (we want to have as few dependencies as possible)
  • I tried to make my code as fast as possible with as few allocations as possible
  • For new code I have written benchmarks so that they can be analyzed and improved

Commit formatting:

Use emojis on commit messages so it provides an easy way of identifying the purpose or intention of a commit. Check out the emoji cheatsheet here: https://gitmoji.carloscuesta.me/

@welcome
Copy link

welcome bot commented Nov 23, 2022

Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@li-jin-gou
Copy link
Contributor

li-jin-gou commented Nov 23, 2022

Add pr Description according to pull request template.

@pjebs
Copy link
Contributor

pjebs commented Nov 30, 2022

Shouldn't the file be cached using sync.Once?

@0xdeface
Copy link
Contributor Author

0xdeface commented Dec 1, 2022

Sorry, I did not understand, what I should do? Use sync.once?

@0xdeface
Copy link
Contributor Author

0xdeface commented Dec 1, 2022

Shouldn't the file be cached using sync.Once?

i didn't change any cache behaviour in this pr. please explain where i should add sync.once?

@ReneWerner87
Copy link
Member

I'll try to take a look at the pull request tomorrow, if I know what he means or have other comments I'll let you know

@pjebs
Copy link
Contributor

pjebs commented Dec 2, 2022

My mistake. I thought the middleware's job was to return a (cached) binary version of the favicon.

@ReneWerner87
Copy link
Member

ReneWerner87 commented Dec 2, 2022

@0xdeface couldn't you save the internal configuration switch by specifying the route when using the middleware ?

example

app.Use("/favicon.svg", favicon.New())

or

app.Use("/favicon.:ext", favicon.New())

you could make this feature much more usable and would only have to deactivate the internal matching in such a case.

@leonklingele
Copy link
Member

Also make sure to update middleware/favicon/README.md accordingly. Currently it says This middleware is exclusively for serving the default, implicit favicon, which is GET /favicon.ico.

@ReneWerner87
Copy link
Member

@0xdeface Hi can check the last comments and suggestions for improvement

hope we can still customize it so that we provide this feature in the next version

@0xdeface
Copy link
Contributor Author

Hi, i will check comments and put changes on next week.

@leonklingele
Copy link
Member

@0xdeface did you get a chance to work on this so far? 😊

Copy link
Member

@renanbastos93 renanbastos93 left a comment

Choose a reason for hiding this comment

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

LGTM, please consider suggestion by @leonklingele

@efectn
Copy link
Member

efectn commented Feb 2, 2023

@leonklingele can you check the PR again

Copy link
Member

@leonklingele leonklingele left a comment

Choose a reason for hiding this comment

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

lgtm, good work @0xdeface! 🥳

@ReneWerner87 ReneWerner87 merged commit 21cd45b into gofiber:master Feb 3, 2023
@welcome
Copy link

welcome bot commented Feb 3, 2023

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants