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

Configurable favicon in config/middleware.json #47

Closed
wants to merge 3 commits into from
Closed

Configurable favicon in config/middleware.json #47

wants to merge 3 commits into from

Conversation

AlbertoGP
Copy link

Setting favicon:"" disables express.favicon() so requests can be served as
static files, otherwise the value is passed on as favicon file path argument.

Setting favicon:"" disables express.favicon() so requests can be served as
static files, otherwise the value is passed on as favicon file path argument.
@tlivings
Copy link
Member

tlivings commented Dec 6, 2013

I'd rather see it be an undefined value rather than an empty string.

@AlbertoGP
Copy link
Author

It needs to be compared to a specific value if we want to keep backwards compatibility: if we use (!config.favicon), not defining a value in the configuration (the default) would remove the favicon() middleware. I assume there is a reason for it to be there as default.

As for the empty string value, in the first version I used plain false which to me made a lot of sense as if saying "do not handle favicons, I'll do that". However, I somehow got confused about the JSON specification when cleaning things up before the pull request and thought that boolean false would not be a valid value; I just checked and it is, so I'll change it back.

as the way to tell Kraken not to use the favicon middleware.
The only reason for switching to an empty string at the last minute
was a misunderstanding on my part of the JSON specification.
@AlbertoGP
Copy link
Author

This feature is covered in a better way by #53.

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.

None yet

2 participants