Skip to content

Conversation

@Martii
Copy link
Member

@Martii Martii commented Jun 18, 2014

  • Make a symlink (soft) to favicon.ico for dev to remove the error of not finding the ico. Affects only dev and /images path should be used for code refs and edits... ignore any "no newline at end of file" messages or it will break
  • Now it will show in dev under Nix... Win7+ or greater is probably required for this to show as found since it handles file symbolic links... XP (EOLd) and before only do dirs.

* Make a symlink (soft) to favicon.ico for dev to remove the error of not finding the ico. Affects only dev and /images path should be used for code refs...ignore any "no newline at end of file" or it will break
* Now it will show in dev under Nix... Win7+ or greater is probably required for this to show as found since it handles file symbolic links...XP and before only do dirs.
@Martii
Copy link
Member Author

Martii commented Jun 18, 2014

Shell Session log:

$ node app.js
Failed to load c++ bson extension, using pure JS version
js-bson: Failed to load c++ bson extension, using pure JS version
GitHub client authenticated
GET / 200 1239ms
GET /css/common.css 200 143ms
GET /css/bootstrap-custom.css 200 153ms
GET /favicon.ico 200 73ms
GET /favicon.ico 304 71ms
GET / 200 337ms
GET /css/bootstrap-custom.css 304 134ms
GET /css/common.css 304 141ms
GET /favicon.ico 304 69ms
GET /?library=true 200 225ms
GET /css/bootstrap-custom.css 304 136ms
GET /css/common.css 304 140ms
GET /favicon.ico 304 69ms
GET /groups 200 219ms
GET /css/bootstrap-custom.css 304 134ms
GET /css/common.css 304 139ms
GET /favicon.ico 304 70ms
...

instead of:

$ node app.js
Failed to load c++ bson extension, using pure JS version
js-bson: Failed to load c++ bson extension, using pure JS version
GitHub client authenticated
GET / 200 1209ms
GET /css/common.css 200 139ms
GET /css/bootstrap-custom.css 200 152ms
GET /favicon.ico 404 73ms
GET /?library=true 200 221ms
GET /css/common.css 304 136ms
GET /css/bootstrap-custom.css 304 142ms
GET /groups 200 215ms
GET /css/bootstrap-custom.css 304 133ms
GET /css/common.css 304 139ms
...

POINT OF INTEREST: Interesting that node.js hides future 404 messages.

@cletusc
Copy link
Contributor

cletusc commented Jun 18, 2014

More curious... why don't we use the favicon middleware (linked SO as it links for each express version)?

@jerone
Copy link
Contributor

jerone commented Jun 18, 2014

for dev to remove the error of not finding the ico

Where do you get 404's? I haven't had one...

@jerone
Copy link
Contributor

jerone commented Jun 18, 2014

More curious... why don't we use the favicon middleware (linked SO as it links for each express version)?

I'm -1 on using another middleware for something so minor. It will only slow things down.

Apparently Express 3 (we are on 3.5.1) has this backed in: http://www.senchalabs.org/connect/favicon.html

app.use(express.favicon(path.join(__dirname, 'public','images','favicon.ico')));

@cletusc
Copy link
Contributor

cletusc commented Jun 18, 2014

app.use(express.favicon(path.join(__dirname, 'public','images','favicon.ico')));

This is what I would use. Either that or actually put the favicon.ico in public/ as opposed to public/images/ that way we don't have an placeholder file. We may as well use Express-supported ways of doing certain things if they provide the means.

@Martii
Copy link
Member Author

Martii commented Jun 18, 2014

Where do you get 404's?

I just get it. Might be a Nix thing.

Apparently Express 3 (we are on 3.5.1) has this backed in: http://www.senchalabs.org/connect/favicon.html

WOW! That's a lot of code to put in for something this small. Who knew? ;) I assume that goes in ./app.js ???

... put the favicon.ico in public/ as opposed to public/images/ that way we don't have an placeholder file.

That could be option 2 but it will make this pr be dependent upon #173 instead (and any future ones referenced if they pop up before merge) of no dependencies. :)

My concern would be if we ever migrate to a newer express will that code break or is it just smoother to do Option 1 in this pr or Option 2 suggested from Cletus? My usual suggestion is lets keep things on the KISS principle... so either of these works for me. I have a bazillion tabs open sometimes so it's nice to know what is what.

@sizzlemctwizzle
Copy link
Member

I assume that goes in ./app.js ???

No it's a one-liner. See b5877be.

My concern would be if we ever migrate to a newer express will that code break

Many things will break when we finally move to Express 4.x. This will be the least of our worries.

@Martii Martii deleted the symlinkToFaviconForDev branch June 18, 2014 21:14
@cletusc
Copy link
Contributor

cletusc commented Jun 18, 2014

For future reference, when we do migrate to Express 4.x, migration would basically change to this instead (per serve-favicon, which is actually for both 3.x and 4.x):

var favicon = require('serve-favicon');
app.use(favicon('public/images/favicon.ico'));

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

enhancement Something we do have implemented already but needs improvement upon to the best of knowledge.

Development

Successfully merging this pull request may close these issues.

4 participants