`twenty-favicon` not loading favicons for some sites

[gitstart-twenty]

Bug Description

There's a bug with twenty-favicon not loading favicons for some sites. This issue is to figure out why this is happening and also fix it.

Example: Refer to https://github.com/twentyhq/twenty/pull/3192#issuecomment-1875070636 for context.

Expected behavior

We should be able to extract favicons on all sites.

[gitstart-app[bot]]

Here is the GitStart Ticket for this issue: https://clients.gitstart.com/twenty/5449/tickets/TWNTY-3219

[gitstart-twenty]

Hey @FelixMalfait @charlesBochet

For some icon types such as ['image/vnd.microsoft.icon', 'image/x-icon'], the buffer generated by ico-to-png could be possibly corrupted / is a mismatch to the buffer expected by sharp. So, sharp is unable to generate metadata from these buffers, throws an error and no icon is returned.

We could skip the conversion to png for these file types, however, the generated icons are very small

The code changes

The icon

However, this is not a very good solution given we don't really know how many icon file types will throw errors.

icojs

Also tried using icojs, but this package keeps throwing the error below:

Error [ERR_REQUIRE_ESM]: require() of ES Module /.../node_modules/icojs/src/node/index.js from /.../dist/favicon/favicon.service.js not supported.
Instead change the require of index.js in /.../dist/favicon/favicon.service.js to a dynamic import() which is available in all CommonJS modules.
    at /.../dist/favicon/favicon.service.js:147:82
    at async FaviconService.getImageFromUrl (/.../dist/favicon/favicon.service.js:147:34) {
  code: 'ERR_REQUIRE_ESM'
}

Changing to a dynamic import with await import('icojs'); does not fix the issue.

[FelixMalfait]

Thanks @gitstart-twenty can't you catch the error and adapt the strategy if there's an error instead of adapting the strategy based on icon type?