Comments (13)
I'm willing to hear other suggestions, but the one here I wouldn't accept. The reason is that before that change to error we had endless issue reports all due to people typoing their path.
from serve-favicon.
Though I can always reconsider depending on what the reason you need to be able to pass in a bad path is.
from serve-favicon.
Ah, ok. Hm, well a generalization of my use case would be: I want to create a web application library which builds on Express. The library should handle favicon stuff out of the box for library users. The library knows /favicon.ico
should resolve to a favicon, but it may or may not have been provided one by the user.
Of course, it could be the library's job to handle the 404 case. But I dunno. Just seems like a sane and pleasant default to not to blow up if you don't have to. It also corresponds to the normal semantic of web servers at large. Request -> Do I have something to serve for that? -> No? -> OK 404. Ya it's a little different, but anyway, that was the behaviour I expected.
But yeah, if you feel the fail fast thing is more important in this case, can't argue!
from serve-favicon.
I'm still thinking about this, which is why I'm still discussing and keeping it open :) So to give a little insight into the reasoning it throws, is because since the only thing this module does is serve up a favicon file, the path is required. One of the key points was that we cached the tiny file in memory and removed all those slow file system stat calls + reads, reducing I/O for this heavily-requested file.
It was thought that without an actual favicon to serve, at that point what would ever be the purpose of this module, since this is an Express/connect middleware and simply not handling the request results in a 404 Not Found.
The idea of blowing up is the same as if, say, this library only accepted a Buffer
and below up if you didn't give it one. This library does accept a Buffer
, but passing a file system path is a convenience method for reading the file in yourself.
from serve-favicon.
I think it's valid to say that if there were no such thing as favicons, this module would be pointless. I'm not sure I agree that the module is pointless when there is no specific favicon to serve. I guess the sense in which I see the module is as a mini server (or object, actor, function, whatever you want to call it) whose sole job is to handle favicon requests. Sure, the server can throw if there is no favicon to serve (well, as long as it's at boot up). But it might be considered an excessively rigid handling of the exception "No favicon found". Not all websites have favicons, after all. Should the servers for all those websites fail to boot unless they have a favicon? Maybe a website has one, then decides to remove it. They have to uninstall the module to boot up again. It's mostly just inconvenient, passing on logic to users which could well be part of its own responsibilities.
A bit more odd in the case of a Buffer, since it seems slightly counter intuitive to pass in null
instead of a Buffer, and not have the thing throw. Like you said, what would it do with an empty buffer? But it's no different than the case of passing in the '/favicon.ico' path. Should there be no file there, or should there be a null passed rather than a Buffer, you're handling the same "exception". The behavior should be identical for both cases.
TLDR: 404ing when no favicon === more robustness (in terms of its responsibility of "serving favicons")
Only downside I see so far is people giving the wrong path and opening issues here, but not sure that's a solvable problem tbh ;)
from serve-favicon.
Would it be possible to gather some other people's opinions in here :)? I just want to be sure there's demand and it makes sense. If justified, I don't mind adding it.
from serve-favicon.
Basically, to sum down the change you're asking for is the following:
Right now, this module says "given a favicon contents, serve it as the favicon". We accept a file path only as a convenience so you do not have to do fs.readFile
and pass us the results.
Instead, your proposal is to change the module to instead of "given a path, serve the favicon at this location".
There are lots of unanswered questions regarding your new approach:
- If the server is still running and you delete the file, does this module start to 404?
- If this module tries to read the file and fails, will it continue to 404 even if you later place the favicon in the correct location?
- If the above is "yes", then how can people do this for
Buffer
s, which were legitimate wants with real use-cases. - And if yes to 1 and 2, how does this module differ from just using
serve-static
module with the single file?
from serve-favicon.
Hmm yah. Bit tricky. I guess in the logical extreme of my suggestion, the answer might be:
- Yes
- Yes
- Er... A setter? Or they couldn't after boot up?
- Uh... The stuff in the README? Convenient default configuration + caching for the favicon special case
But for the sake of simplicity, I might give preference to:
- No
- No
where the justification is: If you're changing your favicon at runtime, your usage is not supported by this module since it's largely meant to provide an in-memory cache of a static resource (should it have such a resource at all)
from serve-favicon.
The stuff in the README? Convenient default configuration + caching for the favicon special case
Answering yes to 1 and 2 removes the only thing this module even does, and at this point, you should use serve-static
for that single file.
If you're changing your favicon at runtime, your usage is not supported by this module since it's largely meant to provide an in-memory cache of a static resource (should it have such a resource at all)
Makes sense. I just fear that as soon as we start giving 404s, people will want it to pick up the changes they are making on disk (like, say, replacing the favicon file with a new one), but idk. Anyway, I'd love to hear someone else chime in, if you can get someone to :)
from serve-favicon.
Hm, alright. Blindly CCing people who were involved with this issue. Sorry people.
@mateodelnorte @jonathanong @sixlettervariables @totherik @aredridel
TLDR: If this middleware isn't given a favicon to serve at startup, should it throw an exception, or keep going and return 404s for favicon requests?
from serve-favicon.
@sfrdmn I have a specific use case that's causing a problem - The favicon is copied into the final destination via gulp.
# Static Assets
app.use favicon("#{BUILD_DIR}/public/favicon.ico", maxAge: 604800000)
app.use sstatic("#{BUILD_DIR}/public")
We don't wait until gulp has finished to start the server. We want to start both the server and build task at the same time (to make everything ready quciker).
Because favicon.ico
probably isnt in the build dir when the server starts, it craps out on us.
It would be helpful if it doesn't crap out on first load if it doesn't exist, if it hasnt been loaded yet, check if it exists, return 404 until it does exist, and when it does exist then cache the file in memory the forever.
I'm not convinced you should be pandering to people putting typo's in their files and wondering why the favicon 404s. Isn't that true for routes and everything else?
from serve-favicon.
I think @joshhunt's approach is valid. I do not see why a whole server should not boot up just because a favicon could not be found. A normal serve-static middleware would also check for the favicon's file on every request. Why not let this middleware do the same until it successfully reads the file.
The benefit of this middleware is in its caching in memory. But not forcing a favicon to exist for server to run.
The next viable option would be to write a module around this one to handle our recurrent work-around.
from serve-favicon.
After much reflection and further discussions on Gitter.im regarding this, the current decision is to leave it as-is. As the module is designed to service a single favicon, either read in from a file or provided by a Buffer
, it is not meant to be constructed without reading in the data.
This module used to be a different name over a year ago, and also back then, we didn't require the favicon to exist on construction. That lead to endless issue reports all due to people typoing their path.
There is a pretty simple solution if you want to dynamically load the favicon.ico file from the path, though: use serve-static
, which serves static files from a file system path. Contrary to belief, serve-static
works just fine to serve a single file (though the file name on the file system has to be the same as the file name in the URL, currently):
var express = require('express')
var serveStatic = require('serve-static')
var app = express()
// serve the favicon file, from __dirname + '/public/favicon.ico'
app.all('/favicon.ico', serveStatic(__dirname + '/public', {
fallthrough: false,
lastModified: false,
maxAge: '1y'
}))
// ... and the rest of your app
Another reason is just unpredictability: using this module you know when your server is running, you will not constantly be hitting the disk to load the favicon file; removing the check will mean a typo or the expected file not existing will cause constant disk queries to return a 404.
In reality, if you want a faivcon, then you use this module. It would be an error if you didn't have the favicon you so clearly desired to have, since you went out of your way to use this module rather than just using default logic to serve it up with a static server. Arguing that the server should just wait until it comes available is the same as you splitting your routes into different JavaScript files and saying if one of those does not exist, the server should still start without those routes and every time a user requests the route, check if the route file exists and 404 until it does. This does not make any sense for most uses, as you probably want to know that your server started and is therefore providing all functionality.
If nothing above is making sense, and you still want this module to 404 until it exists, I suggest someone publishes a wrapper to npm that provides this functionality. Here is some code to get started :)
var createError = require('http-errors')
var fs = require('fs')
var serveFavicon = require('serve-favicon')
module.exports = function (path, options) {
var _middleware
return function (req, res, next) {
if (_middleware) return _middleware(req, res, next)
fs.readFile(path, function (err, buf) {
if (err) return createError(404, err)
_middleware = serveFavicon(buf, options)
return _middleware(req, res, next)
})
}
}
from serve-favicon.
Related Issues (20)
- Purpose HOT 3
- Performance HOT 2
- where do i get the favicon icon ? HOT 1
- Support Requirement for Multiple Favicons HOT 4
- doc typo HOT 1
- serving different favicon for different pages HOT 1
- Possible to use location outside of your repo? HOT 4
- Can't see favicon when using Microsoft Edge HOT 2
- Vary favicon by domain HOT 4
- Doubts on res._headers HOT 3
- Favicon not showing when running server with long context path. HOT 5
- How to use png favicon files? HOT 1
- how can I use a url instead of path? HOT 4
- how to add different favicon for different routers? HOT 3
- serve-favicon vulnerable to RegEx Denial of Service attack HOT 1
- Favicon is not showing HOT 5
- Can't run express js server app on my local host:3000 HOT 4
- Feature Request: Serve icon based on host mapping HOT 2
- Doesn't work with Chrome HOT 4
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from serve-favicon.