feat: Create a favicon for an artist based on their avatar #912

lenikadali · 2024-11-21T19:11:16Z

Fixes #330

Adds sharp-ico package, using that library to create the artist favicon from the artist avatar

netlify · 2024-11-21T19:11:19Z

👷 Deploy request for mirlo pending review.

Name	Link
🔨 Latest commit	`107daa2`

simonv3

Amazing, thanks for doing this!

Added a comment that i think should help

src/jobs/optimize-image.ts

simonv3

This looks good! I can't really think of a way to make the final folder structure a bit more organized. I wonder if favicons should go into their own bucket, but I guess that doesn't necessarily have to happen? As long as the file name is consistent and identifiable.

simonv3 · 2024-12-05T15:17:32Z

@lenikadali not sure why this is failing. I'm wondering if you could try rebasing your branch onto the main branch? (or simply merging it) and then re-running yarn install?

lenikadali · 2024-12-09T18:42:49Z

This looks good! I can't really think of a way to make the final folder structure a bit more organized. I wonder if favicons should go into their own bucket, but I guess that doesn't necessarily have to happen? As long as the file name is consistent and identifiable.

I would lean towards a separate bucket for favicons. That being said, I think it would (potentially) require moving the whole process to utils. Maybe make another issue to cover that as follow up work?

Added the sharp-ico package and tried to generate a favicon based on the artist avatar we have.

Fixed comment to match JS style comments

Added call to upload the artist avatar favicon to the Minio store and some logging to indicate when it happens

Removed the artistFavicon variable since the return value of the ico method is the size, width and height properties of the favicon. Switch to setting the file name as a variable and use that when saving the favicon to the Minio bucket.

lenikadali · 2024-12-09T18:48:40Z

@lenikadali not sure why this is failing. I'm wondering if you could try rebasing your branch onto the main branch? (or simply merging it) and then re-running yarn install?

Did this and yarn install didn't seem to change package.json

simonv3 · 2024-12-09T22:41:21Z

Hmm, I'll pull down the branch and try to replicate in the next day or so to try and see what's up.

simonv3 · 2024-12-10T14:53:18Z

Odd, yarn tsc passes on main, but not on this branch. I'm wondering whether sharp-ico introduces a faker version somewhere? I tried upgrading to a more recent version of typescript and the error still persists. Specifying where types are stored also doesn't help.

lenikadali · 2024-12-10T16:43:11Z

yarn.lock

-"@faker-js/faker@npm:^8.3.1":
-  version: 8.3.1
-  resolution: "@faker-js/faker@npm:8.3.1"
-  checksum: 10/66b25cb82af907ff127767b54a5d6e95919c8051cd2de9131a6f5dd45dc539ebfbafad66e590e603e347e4c411f0f83c28e70043024af66faebba49d9868c263
+"@esbuild/win32-x64@npm:0.24.0":
+  version: 0.24.0
+  resolution: "@esbuild/win32-x64@npm:0.24.0"
+  conditions: os=win32 & cpu=x64
  languageName: node
  linkType: hard

-"@fal-works/esbuild-plugin-global-externals@npm:^2.1.2":
-  version: 2.1.2
-  resolution: "@fal-works/esbuild-plugin-global-externals@npm:2.1.2"
-  checksum: 10/fd68714cccfbd33a8ec31d11ac7c6373100a5e1b8e31941a45c723c802feccb0a00dde946f55cc91d58bff77d405adc2064b22f0faf5ee165968965e5da758a1
+"@faker-js/faker@npm:^8.3.1":
+  version: 8.4.1
+  resolution: "@faker-js/faker@npm:8.4.1"
+  checksum: 10/5983c2ea64f26055ad6648de748878e11ebe2fb751e3c7435ae141cdffabc2dccfe4c4f49da69a3d2add71e21b415c683ac5fba196fab0d5ed6779fbec436c80
  languageName: node


Odd, yarn tsc passes on main, but not on this branch. I'm wondering whether sharp-ico introduces a faker version somewhere? I tried upgrading to a more recent version of typescript and the error still persists. Specifying where types are stored also doesn't help.

@simonv3 it would seem it does going by the changes in yarn.lock here. Let me take a look at the library's dependencies to see if we can get one that depends on main's version of faker.

I tried to go by the versions here and tried yarn install for all of them. yarn.lock didn't change the faker version back to what it is on main so it looks like the newer version was the original dependency for sharp-ico.

Another thing we could do is freeze the faker version to 8.3.1. The diff would look like so:

$ git diff diff --git a/package.json b/package.json index 1dd8dc92..18f57d25 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,7 @@ }, "dependencies": { "@bull-board/express": "^4.11.0", - "@faker-js/faker": "^8.3.1", + "@faker-js/faker": "8.3.1", "@prisma/client": "5", "@types/connect-busboy": "^1.0.3", "@types/faker": "^6.6.9", diff --git a/yarn.lock b/yarn.lock index 504574d0..d4c4365e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1755,10 +1755,10 @@ __metadata: languageName: node linkType: hard -"@faker-js/faker@npm:^8.3.1": - version: 8.4.1 - resolution: "@faker-js/faker@npm:8.4.1" - checksum: 10/5983c2ea64f26055ad6648de748878e11ebe2fb751e3c7435ae141cdffabc2dccfe4c4f49da69a3d2add71e21b415c683ac5fba196fab0d5ed6779fbec436c80 +"@faker-js/faker@npm:8.3.1": + version: 8.3.1 + resolution: "@faker-js/faker@npm:8.3.1" + checksum: 10/66b25cb82af907ff127767b54a5d6e95919c8051cd2de9131a6f5dd45dc539ebfbafad66e590e603e347e4c411f0f83c28e70043024af66faebba49d9868c263 languageName: node linkType: hard @@ -13044,7 +13044,7 @@ __metadata: resolution: "mirlo@workspace:." dependencies: "@bull-board/express": "npm:^4.11.0" - "@faker-js/faker": "npm:^8.3.1" + "@faker-js/faker": "npm:8.3.1" "@prisma/client": "npm:5" "@types/archiver": "npm:^5.3.2" "@types/bcryptjs": "npm:^2.4.2"

@lenikadali does freezing it prevent the type error?

No unfortunately not.

This is what I see when I run yarn tsc:

$ yarn tsc error TS2688: Cannot find type definition file for 'faker'. The file is in the program because: Entry point for implicit type library 'faker' Found 1 error.

The other thing we could try is cherry-pick the commits onto main and see if yarn tsc passes (with the freeze).

Edit: tried to cherry-pick and yarn tsc still fails. It seems that for this to be merged, we may have to upgrade faker.

does upgrading faker help? I thought I did that when I was attempting this locally earlier in the week, but maybe that's a step I forgot. It's totally fine to upgrade faker.

I tried upgrading faker and that didn't seem to do anything.

weird because the dependency tree of sharp-ico does not include faker:

└─ [email protected] ├─ @types/[email protected] │ └─ @types/[email protected] │ └─ [email protected] ├─ [email protected] │ ├─ [email protected] │ ├─ [email protected] │ │ ├─ [email protected] │ │ └─ @canvas/[email protected] │ └─ @canvas/[email protected] ├─ [email protected] └─ [email protected] ├─ [email protected] ├─ @img/[email protected] │ └─ @emnapi/[email protected] │ └─ [email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ [email protected] │ ├─ [email protected] │ │ └─ [email protected] │ └─ [email protected] │ ├─ [email protected] │ │ └─ [email protected] │ └─ [email protected] ├─ [email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] │ └─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] ├─ @img/[email protected] └─ @img/[email protected]

Okay, I think something else happened here. When I go to the main branch, create a new branch for this, and install sharp-ico from scratch, I don't get the type error.

@lenikadali I wonder if starting from scratch here would be a good idea? Basically just a blank branch, install sharp-ico and then add the lines changed. I'd do the copy paste job but want to make sure you get credit!

Hi @simonv3 that's a good idea. New PR is here

If the pipeline passes for that one, this PR can be closed 🤞

Re-doing the changes from the original PR funmusicplace#912 adding the ability to create a favicon from an artist's avatar. Somehow faker got upgraded and made tests fail

simonv3 reviewed Nov 21, 2024

View reviewed changes

src/jobs/optimize-image.ts Outdated Show resolved Hide resolved

lenikadali marked this pull request as ready for review November 27, 2024 16:58

lenikadali commented Nov 27, 2024

View reviewed changes

src/jobs/optimize-image.ts Outdated Show resolved Hide resolved

simonv3 approved these changes Dec 5, 2024

View reviewed changes

simonv3 self-requested a review December 5, 2024 01:34

simonv3 approved these changes Dec 5, 2024

View reviewed changes

lenikadali added 5 commits December 9, 2024 21:45

Add sharp-ico package, try to make artist avatar favicon

83cfb6c

Added the sharp-ico package and tried to generate a favicon based on the artist avatar we have.

Fix comment

ad9268a

Fixed comment to match JS style comments

Upload favicon to Minio store

3453e10

Added call to upload the artist avatar favicon to the Minio store and some logging to indicate when it happens

Make sure favicon name is unique

107daa2

lenikadali force-pushed the 330-create-favicon-for-artist-based-on-their-avatar branch from 764fdcb to 107daa2 Compare December 9, 2024 18:46

lenikadali commented Dec 10, 2024

View reviewed changes

lenikadali mentioned this pull request Dec 13, 2024

feat: Create a favicon for an artist based on their avatar #937

Merged

simonv3 closed this Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Create a favicon for an artist based on their avatar #912

feat: Create a favicon for an artist based on their avatar #912

lenikadali commented Nov 21, 2024

netlify bot commented Nov 21, 2024 •

edited

Loading

simonv3 left a comment

simonv3 left a comment •

edited

Loading

simonv3 commented Dec 5, 2024

lenikadali commented Dec 9, 2024 •

edited

Loading

lenikadali commented Dec 9, 2024

simonv3 commented Dec 9, 2024

simonv3 commented Dec 10, 2024 •

edited

Loading

lenikadali Dec 10, 2024

lenikadali Dec 10, 2024

lenikadali Dec 10, 2024 •

edited

Loading

simonv3 Dec 10, 2024

lenikadali Dec 11, 2024 •

edited

Loading

simonv3 Dec 11, 2024

simonv3 Dec 12, 2024 •

edited

Loading

simonv3 Dec 12, 2024

lenikadali Dec 13, 2024

feat: Create a favicon for an artist based on their avatar #912

feat: Create a favicon for an artist based on their avatar #912

Conversation

lenikadali commented Nov 21, 2024

netlify bot commented Nov 21, 2024 • edited Loading

👷 Deploy request for mirlo pending review.

simonv3 left a comment

Choose a reason for hiding this comment

simonv3 left a comment • edited Loading

Choose a reason for hiding this comment

simonv3 commented Dec 5, 2024

lenikadali commented Dec 9, 2024 • edited Loading

lenikadali commented Dec 9, 2024

simonv3 commented Dec 9, 2024

simonv3 commented Dec 10, 2024 • edited Loading

lenikadali Dec 10, 2024

Choose a reason for hiding this comment

lenikadali Dec 10, 2024

Choose a reason for hiding this comment

lenikadali Dec 10, 2024 • edited Loading

Choose a reason for hiding this comment

simonv3 Dec 10, 2024

Choose a reason for hiding this comment

lenikadali Dec 11, 2024 • edited Loading

Choose a reason for hiding this comment

simonv3 Dec 11, 2024

Choose a reason for hiding this comment

simonv3 Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

simonv3 Dec 12, 2024

Choose a reason for hiding this comment

lenikadali Dec 13, 2024

Choose a reason for hiding this comment

netlify bot commented Nov 21, 2024 •

edited

Loading

simonv3 left a comment •

edited

Loading

lenikadali commented Dec 9, 2024 •

edited

Loading

simonv3 commented Dec 10, 2024 •

edited

Loading

lenikadali Dec 10, 2024 •

edited

Loading

lenikadali Dec 11, 2024 •

edited

Loading

simonv3 Dec 12, 2024 •

edited

Loading