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

favicon.ico files are all broken #688

Closed
huochunpeng opened this Issue Aug 8, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@huochunpeng
Member

huochunpeng commented Aug 8, 2017

I'm submitting a bug report

  • Library Version:
    0.30.1

Please tell us about your environment:

  • Operating System:
    OSX 10.x

  • Node Version:
    All

  • NPM Version:
    All

  • Browser:
    all

  • Language:
    all

Current behavior:
Did anyone notice the favicon.ico files are all broken?

./lib/resources/content/favicon.ico
./lib/resources/content/javascriptservices/wwwroot/favicon.ico
./lib/resources/img/favicon.ico 

I cannot view the files locally.

Browser also reports the file has error when viewing deployed app.

Expected/desired behavior:
Not an issue on production app, since every app will replace the default favicon. But broken file is just annoying.

  • What is the expected behavior?

  • What is the motivation / use case for changing the behavior?

@JeroenVinke

This comment has been minimized.

Member

JeroenVinke commented Aug 8, 2017

Hey @huochunpeng.

Where is this part coming from?

./lib/resources/content/favicon.ico
./lib/resources/content/javascriptservices/wwwroot/favicon.ico
./lib/resources/img/favicon.ico

Also, could you tell me what the error is that you get in the browser?

@JeroenVinke JeroenVinke added the bug label Aug 8, 2017

@huochunpeng

This comment has been minimized.

Member

huochunpeng commented Aug 8, 2017

These are the files location in cli source code. One of the favicon.ico is copied to app root directory when au new app creats the app.

The favicon.ico in the bootstrapped app can not be viewed in any tool.
When viewing index page in browser http://localhost:9000 , browser shows no icon even after I added <link rel="icon" href="favicon.ico" type="image/x-icon"> to the html head.

Manual visit at http://localhost:9000/favicon.ico yields broken file. Firefox says cannot be displayed because it has errors.

@JeroenVinke

This comment has been minimized.

Member

JeroenVinke commented Aug 8, 2017

Oh yeah, I see the error in firefox now too when I try and open the favicon. Would you like to create a PR and update the favicons with this one?

@huochunpeng

This comment has been minimized.

Member

huochunpeng commented Aug 8, 2017

Thanks for the invitation. But I am too lazy for a PR, not that obsessed to show my name in the git log.

It would be easier for you to just commit directly.

Need to add <link rel="icon" type="image/x-icon" href="favicon.ico" /> <link rel="icon" type="image/png" href="favicon.ico" />(your favicon is not ico format, but png format) to html head, it looks like html5 needs this explicitly.

Note, the existing broken favicons are png format too, but they are still broken png files.

@diegohb

This comment has been minimized.

diegohb commented Aug 11, 2017

may consider using this: http://realfavicongenerator.net/

@JeroenVinke

This comment has been minimized.

Member

JeroenVinke commented Aug 12, 2017

Thank you @huochunpeng and @diegohb. I have created a PR here: #691

@huochunpeng

This comment has been minimized.

Member

huochunpeng commented Mar 7, 2018

@JeroenVinke this issue need to be reopened.

App created by cli still has broken favicon.ico file. The files in cli repo are good, sadly they are not the real issue.

The real issue is

return fs.readFile(this.sourcePath).then(data => {

That line then use

fs.readFile(path, encoding || 'utf8', (error, data) => {

To process all files in utf8 encoding.

The result is copied favicon.ico is 24k in every app, while original file in cli repo is 15k.

We need another api on ProjectItem to support copying binary file which should use default encoding null. I will compose a PR in few days.

@Alexander-Taran

This comment has been minimized.

Member

Alexander-Taran commented Mar 7, 2018

aren't there fs.copy or something?
ignore me.. haven't read a lot of cli source. don't know the assumptions behind

@JeroenVinke JeroenVinke reopened this Mar 7, 2018

@huochunpeng

This comment has been minimized.

Member

huochunpeng commented Mar 7, 2018

@Alexander-Taran thx. I will try to refactor the code to use fs.copyFile which makes more sense.

@Alexander-Taran

This comment has been minimized.

Member

Alexander-Taran commented Mar 8, 2018

@huochunpeng while you at it, may as well check this one?
#828

huochunpeng added a commit to huochunpeng/cli that referenced this issue Mar 10, 2018

fix: fix binary file copy on favicon.ico
Set encoding:null on binary file resource.
fs.readFile returns raw buffer.
fs.writeFile ignores our default 'utf8' encoding when
incoming data is a buffer.

Closes aurelia#688.
@huochunpeng

This comment has been minimized.

Member

huochunpeng commented Mar 10, 2018

Could not use fs.copyFile due to fileExistsStrategy.

@Alexander-Taran

This comment has been minimized.

Member

Alexander-Taran commented Mar 25, 2018

@JeroenVinke PR waiting to fix favicon

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