Favicons 4 about favicons HOT 26 CLOSED

Let's assume ImageMagick and GM are out of the question due to their age and dependencies. Remove Pica as (from the perspective of a JS-only library) JIMP appears more popular. So either Sharp or JIMP I believe. As per Sindre's comment:

It will be somewhat slower than native, but can be fixed caching. Having to install dependencies are way worse.

I think JIMP is the way to go.

from favicons.

I never used JIMP, but it make sense to use pure JS. Note that, if you want to support the new Safari pinned tabs, you'll need to generate SVG, which JIMP apparently doesn't support.

from favicons.

From my perspective as a library author consuming this library as a dependency, I'd love to see a couple things in particular:

1. Consistent error handling. Right now the user supplied callback can be invoked in multiple ways (node err style callbacks, simple result callbacks with no error info), with varying types of error reporting (error objects, simple strings).
2. Definitely 👍 on ditching the file writing. I'd say focus on one thing, and doing that thing well. In this library's case, I'd say that means encapsulating the cross-browser favicon differences. I'd vote to leave the file writing, index.html injection, etc to build tools downstream.

from favicons.

I would love to see a more sane config format with general configuration options and vendor specific sections.

I also agree with @davewasmer about the file-writing - that was my major reason for requesting (and implementing) extra options - I needed them solely to configure how the manifest.json file was output and this really has nothing to do with favicons!

from favicons.

@davewasmer Good idea. I think the Favicons callback will look like this:

var cheerio = require('cheerio');

function (error, images, html) {
    // error: any error that occurred in the process (string)
    if (error) throw error;

    // images: an object of buffers e.g. { apple-touch-icon.png: <Buffer>, favicon.ico: <buffer> }
    for (var favicon in images) {
        if (images.hasOwnProperty(favicon)) {
            fs.writeFileSync('dist/' + favicon, images[favicon]);
        }
    }

    // html: a snippet of HTML (string) e.g.
    $ = cheerio.load('index.html', { decodeEntities: false });
    $('head').append(html);
}

I definitely agree on adding this to the build tools but it seems like quite a bit of work for the Node module. Maybe we can have an optional write?

Gulp will be something like:

gulp.task('default', function () {
    gulp.src('logo.jpg')
        .pipe(favicons({
            background: '#1d1d1d'   // Normal variables
            html: 'index.html'      // Gulp-specific variables
        }))
        .pipe(gulp.dest('favicons/'));
});

from favicons.

@euangoddard My current concept for the configuration (with defaults) is this:

{
  "appName": null,
  "appDescription": null,
  "developer": null,
  "developerURL": null,
  "background": "#fff",
  "path": "./",
  "silhouette": false,
  "version": 1.0,
  "logging": false,
  "online": false,
  "icons": {
    "android": true,
    "appleIcon": true,
    "appleStartup": true,
    "coast": true,
    "favicons": true,
    "firefox": true,
    "opengraph": true,
    "windows": true,
    "yandex": true
  }
}

Not entirely sure on the specifics like "silhouette" as it depends on the final output, but what are your thoughts on this?

from favicons.

@haydenbleasel Looks great. A couple suggestions, off the top of my head:

    // error: any error that occurred in the process (string)
    if (error) throw error;

I'd favor an Error subclass over a string, or at the very least an object. Makes it easier to add parseable metadata to the error in the future.

For example, if I elect to use the API, and the API returns an error like Picture isn't square!, a string error my read Request failed: 400 - Picture isn't square. But for my build tool, I don't want to expose the HTTP status code or "Request failed" bit to my end user - they aren't thinking in terms of HTTP errors, they just installed this ember-cli-favicon thing. I just want them to see "Picture isn't square!", or better yet, I want to detect that particular error condition and provide my own error message to them ("Go change your public/favicon.png to be square").

    // images: an object of buffers e.g. { apple-touch-icon.png: <Buffer>, favicon.ico: <buffer> }
    for (var favicon in images) {
        if (images.hasOwnProperty(favicon)) {
            fs.writeFileSync('dist/' + favicon, images[favicon]);
        }
    }

Personally, I think an array of { buffer, filename } objects might be better here - most likely, I'm not going to be manually selecting individual images by their hardcoded filename (i.e. doSomethingWIth(favicon['apple-touch-icon.png'])), but rather I'll just be iterating over them all.

I definitely agree on adding this to the build tools but it seems like quite a bit of work for the Node module...

Not sure what you mean here - that your callback snippet is too much work? I think that is fine - I'm guessing most people wouldn't consume this library directly (but I'm admitted biased here, as a consuming lib author 😉)

from favicons.

@davewasmer Good idea on the errors, is there a specific error object style I should use? Otherwise I'm thinking { status: <number>, error: <string>, message: <string> }. I'd like to provide useful error messages like "Go change your public/favicon.png to be square" natively.

As for the images callback, I suppose there's not much difference with the following except for simpler syntax:

images.forEach(function (favicon) {
    fs.writeFileSync('dist/' + favicon.name, favicon.contents);
});

from favicons.

@haydenbleasel for the structure, I don't think it matters a ton, as long as (a) it's consistent to whatever extent possible (i.e. if it's a filesystem error, I wouldn't expect an HTTP status, but I'd expect a message no matter what), and (b) it's easy to inspect for certain error conditions or groups of conditions (i.e. all errors have error: <enum>, where valid values are things like 'APIError', 'FSError', 'ValidationError', etc). Just suggestions.

from favicons.

@davewasmer Okay sounds good, here's the error template we'll be using:

{
    status,    // HTTP Status Code or null if filesystem error
    error,     // Enum or title, to be discussed in detail
    message    // Description of the error, fallback to "An unknown error has occurred"
}

from favicons.

@haydenbleasel that config is so much clearer to me - a big win. My only comment would be that runtime configuration is mixed with outcome-based configuration (i.e.logging and appName sit at the same level). However, I think creating sub-sections for these probably isn't worth it.

from favicons.

@euangoddard Yeah the current favicons config is categorised but that causes too much work for simple parameters e.g. Basic configuration and disabling 1 icon type:

{
    files: {
        src: a,
        dest: b,
        html: c
    },
    icons: {
        appleIcon: false
    },
    settings: {
        appName: d,
        appDescription: e,
        developer: f,
    }
}

New one:

{
    appName: d,
    appDescription: e,
    developer: f,
    html: c,
    icons: {
        appleIcon: false
    }
}

from favicons.

Does anyone know if there's a pure Javascript npm package for convering SVG to a rasterized image type like JPG / PNG / BMP?

from favicons.

Although it's a bit strange to me, the new concept for files is to return something like:

{
    name: 'browserconfig.xml',
    contents: [
        '<square70x70logo src="small.png"/>',
        '<square150x150logo src="medium.png"/>',
        '<wide310x150logo src="wide.png"/>',
        '<square310x310logo src="large.png"/>',
        '<TileColor>#009900</TileColor>'
   ]
}

That way it's consistent with the way I return HTML (just the contents, no wrapper). In the Node module, the user is left to handle that output and append it to an existing browserconfig.xml file. In the Gulp plugin, it creates the browserconfig.xml file for you.

from favicons.

@phbernard I think we need proper unit tests for this new version, you any good with that?

from favicons.

Yup! Don't count on me to write hundreds of tests for every aspects of the module, but I can surely write some to get started. Which parts would you like to focus on?

from favicons.

Haha yeah of course mate :) I guess we're generating 3 seperate components at the moment (images, files, html) so maybe just checking that the correct files have been created / the correct HTML has been produced? What do you reckon?

from favicons.

Sounds good. I'll code some mocha tests, I think tomorrow.

from favicons.

Sounds good! Thanks mate :)

from favicons.

@haydenbleasel looking at that output - are you saying that the file contents will be an array of strings?

from favicons.

Hey @davewasmer, updated callback is as follows:

function (error, response) {
    console.log(error.status); // HTTP error code or null
    console.log(error.name); // e.g. "API Error"
    console.log(error.message); // e.g. An unknown error has occurred
    console.log(response.images); // Array of { name: string, contents: <buffer> }
    console.log(response.files); // Array of { name: string, contents: <buffer> }
    console.log(response.html); // Array of strings (html elements)
});

from favicons.

ah, makes sense 👍

from favicons.

Okay so I'm working on the new RFG integration now, there's two ways of handling the image/file response cc: @davewasmer @phbernard. The first is the traditional way, we'd have to request the zip file, unzip it and pass the files into an array or something:

favicon_generation_result.favicon.package_url = 'http://realfavicongenerator.net/files/.../favicons.zip'

The second is downloading each image individually, decreasing request size and avoiding the whole unzipping process but increases the amount of HTTP requests:

favicon_generation_result.favicon.files_urls: [
  'http://realfavicongenerator.net/files/../package_files/android-chrome-144x144.png',
  'http://realfavicongenerator.net/files/../package_files/android-chrome-192x192.png',
  'http://realfavicongenerator.net/files/../package_files/android-chrome-36x36.png',
  'http://realfavicongenerator.net/files/../package_files/android-chrome-48x48.png',
  'http://realfavicongenerator.net/files/../package_files/android-chrome-72x72.png',
  'http://realfavicongenerator.net/files/../package_files/android-chrome-96x96.png',
  ...

Thoughts?

from favicons.

I'd vote for the latter (separate requests). Since this is a Node lib, it won't be subject to the browser's concurrency restrictions, so firing off a many simultaneous requests shouldn't be a problem. On the other hand saving the bandwith and avoiding the whole unzipping problem seems very attractive.

Sent from my iPhone

On Nov 9, 2015, at 10:18 PM, Hayden Bleasel [email protected] wrote:

Okay so I'm working on the new RFG integration now, there's two ways of handling the image/file response cc: @davewasmer @phbernard. The first is the traditional way, we'd have to request the zip file, unzip it and pass the files into an array or something:

favicon_generation_result.favicon.package_url = 'http://realfavicongenerator.net/files/.../favicons.zip'
The second is downloading each image individually, decreasing request size and avoiding the whole unzipping process but increases the amount of HTTP requests:

favicon_generation_result.favicon.files_urls: [
'http://realfavicongenerator.net/files/../package_files/android-chrome-144x144.png',
'http://realfavicongenerator.net/files/../package_files/android-chrome-192x192.png',
'http://realfavicongenerator.net/files/../package_files/android-chrome-36x36.png',
'http://realfavicongenerator.net/files/../package_files/android-chrome-48x48.png',
'http://realfavicongenerator.net/files/../package_files/android-chrome-72x72.png',
'http://realfavicongenerator.net/files/../package_files/android-chrome-96x96.png',
...
Thoughts?

—
Reply to this email directly or view it on GitHub.

from favicons.

@davewasmer Agreed. We can asynchronously grab each one of the files, should also avoid timeout issues and finish quicker.

from favicons.

Also @phbernard I totally forgot to respond to that last message re: SVG support for El Capitan, see #53 as I'm not sure we can get it into 4.0.0.

from favicons.