refactor(image): deprecate avatarLegacy for removal #3109

ST-DDT · 2024-09-14T14:23:21Z

Makes #2934 obsolete

Prefer a stable CDN over IPFS #2934

The faker.image.avatarLegacy() method no longer produces useful results, so we should remove it soon.

Theoretically, we could just remove it, but I'm not sure that is a good solution since it is technically a breaking change.

https://deploy-preview-3109.fakerjs.dev/api/image.html#avatarlegacy

netlify · 2024-09-14T14:23:38Z

✅ Deploy Preview for fakerjs ready!

Name	Link
🔨 Latest commit	`fedd121`
🔍 Latest deploy log	https://app.netlify.com/sites/fakerjs/deploys/66ec8e46a94b1f0008d1e512
😎 Deploy Preview	https://deploy-preview-3109.fakerjs.dev
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-09-14T14:26:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (f27f9c5) to head (fedd121).
Report is 1 commits behind head on next.

Additional details and impacted files

@@           Coverage Diff           @@
##             next    #3109   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files        2776     2776           
  Lines      226338   226341    +3     
  Branches      937      944    +7     
=======================================
+ Hits       226253   226268   +15     
+ Misses         85       73   -12

Files with missing lines	Coverage Δ
src/modules/image/index.ts	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

matthewmayer · 2024-09-14T15:41:29Z

Could we make it an alias to avatar for the time being?

xDivisionByZerox · 2024-09-14T19:10:12Z

Could we make it an alias to avatar for the time being?

I wouldn't do that, since that takes away some of the incentive to actually migrate.

matthewmayer · 2024-09-14T19:40:30Z

Shipping a version with urls we know to be broken doesn't seem ideal either though.

ST-DDT · 2024-09-14T20:05:38Z

If someone uses the legacy method, they very intentionally used that method because they wanted that image set and not the GH avatars.
I also considered throwing an error instead but that would break other peoples applications.
Not having images will hopefully bring them to investigate why and replace the method.

I agree that this situation is not ideal, but I don't see a solution that wouldn't break our tests or specification.

We should add a test, to ensure that our generated links are still valid (if they are expected to be).

Shinigami92 · 2024-09-14T20:18:49Z

Right now, these would e.g. "fail" with an

when used for demo apps

And this method is used in my previous company. I could ask/notify a friend there the next few days and tell them, that they need to find a new way to fake human faces.

ST-DDT · 2024-09-14T20:24:52Z

And this method is used in my previous company. I could ask/notify a friend there the next few days and tell them, that they need to find a new way to fake human faces.

Potential Workaround:

faker.image.urlLoremFlickr({ category: 'portrait', width: 400, height: 400 })

Shinigami92 · 2024-09-14T20:29:18Z

And this method is used in my previous company. I could ask/notify a friend there the next few days and tell them, that they need to find a new way to fake human faces.

Potential Workaround:
faker.image.urlLoremFlickr({ category: 'portrait', width: 400, height: 400 })

Nah, this wont work for the data they need. They need real human faces. Like ID card pictures.

And this.... is not something like that XD

Details

matthewmayer · 2024-09-15T11:50:15Z

We could consider pointing to a static image with a message that avatarLegacy is deprecated? More useful than a broken image.

matthewmayer · 2024-09-15T11:51:58Z

I'd like to get a basic version of #465 implemented for 9.1.0 so we have a more useful replacement to point people to.

Shinigami92 · 2024-09-15T11:53:11Z

We could consider pointing to a static image with a message that avatarLegacy is deprecated? More useful than a broken image.

Sounds like a quick good work around for the moment.

I'd like to get a basic version of #465 implemented for 9.1.0 so we have a more useful replacement to point people to.

Not sure if 9.1, but potentially a 9.X.

ST-DDT · 2024-09-16T08:28:25Z

We could consider pointing to a static image with a message that avatarLegacy is deprecated? More useful than a broken image.

Sounds like a quick good work around for the moment.

Which image should I use instead?

Something like this?

Shinigami92 · 2024-09-16T08:41:42Z

Which image should I use instead?

Something like this?

Partly yes, but no...
faker.image.avatar() does not solve the same purpose and is more targeted for "just" internet profile pictures => avatars
So maybe faker.image.avatarLegacy() is broken, pls help/contribute

ST-DDT · 2024-09-16T08:59:58Z

So maybe faker.image.avatarLegacy() is broken, pls help/contribute

Maybe like this?

faker.image.avatarLegacy() is borken
use faker.image.avatar() instead
or consider contributing to #465

Shinigami92 · 2024-09-16T09:04:17Z

So maybe faker.image.avatarLegacy() is broken, pls help/contribute

Maybe like this?

faker.image.avatarLegacy() is borken
use faker.image.avatar() instead
or consider contributing to #465

could be a compromise

maybe use faker.image.avatar() instead for PFP (https://www.dictionary.com/e/acronyms/pfp/)

ST-DDT · 2024-09-16T09:11:46Z

maybe use faker.image.avatar() instead for PFP (dictionary.com/e/acronyms/pfp)

The fact that you have to put a dictionary there kind of disqualifies that abbreviation.
IMO the term avatar is well understood and more known than PFP.

matthewmayer · 2024-09-16T13:59:48Z

Maybe open a new GH discussion and just embed the full URL to that discussion in the image. Easier to edit that with a longer description of possible alternatives than trying to squeeze it into an image.

matthewmayer · 2024-09-17T14:07:26Z

I discovered the images are still available on an alternative IPFS endpoint at https://gateway.pinata.cloud/ipfs/Qmd3W5DuhgHirLHGVixi6V76LhCkZUz6pnFt5AJBiyvHye/avatar/ if we want to retrieve the images from there.

ST-DDT · 2024-09-17T14:57:15Z

I discovered the images are still available on an alternative IPFS endpoint at https://gateway.pinata.cloud/ipfs/Qmd3W5DuhgHirLHGVixi6V76LhCkZUz6pnFt5AJBiyvHye/avatar/ if we want to retrieve the images from there.

Those images have too many issues. We wont bring them back.

ST-DDT · 2024-09-19T15:12:43Z

Team Decision

We will deprecate the method without changing the target urls.

If we replace the generated links with a custom image, we have to host that image somewhere FOREVER
We don't plan on backporting the fix to older methods either, it will just provide a link that is no longer working.
The method will not throw an error itself in case somebody only needs the link (the string value)

Shinigami92

I would like to add, that I was at a conference while the "team decision" was made.

src/modules/image/index.ts

Shinigami92 · 2024-09-20T16:47:42Z

src/modules/image/index.ts

    return `https://cloudflare-ipfs.com/ipfs/Qmd3W5DuhgHirLHGVixi6V76LhCkZUz6pnFt5AJBiyvHye/avatar/${this.faker.number.int(
      1249
    )}.jpg`;


suggestion: @matthewmayer had a potentially good idea here: #3109 (comment)
Should we consider it? If not, why?

Are you referring to the discussion or the image/the link within?

The link. So the idea is to return 'https://github.com/faker-js/faker/discussions/XXXX'; where XXXX is the new discussion.
This will still result then in a missing image tag.

e.g.:

Changing the current link breaks our tests and might be breaking for some of our users.
This isn't meant as a strict rejection, just a reminder that every change breaks someones workflow (similar to how avatar() would break the usecase for needing portraits, even though it isn't defined anywhere).
That's one of the reasons, why I would to be very specific with the expectations we have for the results of a method.
(aka should we provide metadata or would these serve a purpose not aligned with the method that serves the images)

https://xkcd.com/1172/

On the other hand, if the users don't even look at our docs and their own logs, why would they read a random link in the browser?

Did not considered these points 👍👌
So we could then instead throw a runtime warning with a link 🤔

refactor(image): deprecate avatarLegacy

6422118

ST-DDT added p: 1-normal Nothing urgent c: refactor PR that affects the runtime behavior, but doesn't add new features or fixes bugs m: image Something is referring to the image module deprecation A deprecation was made in the PR labels Sep 14, 2024

ST-DDT added this to the vAnytime milestone Sep 14, 2024

ST-DDT requested review from a team September 14, 2024 14:23

ST-DDT self-assigned this Sep 14, 2024

ST-DDT linked an issue Sep 14, 2024 that may be closed by this pull request

Prefer a stable CDN over IPFS #2934

Closed

10 tasks

ST-DDT mentioned this pull request Sep 14, 2024

Prefer a stable CDN over IPFS #2934

Closed

10 tasks

xDivisionByZerox approved these changes Sep 14, 2024

View reviewed changes

ST-DDT mentioned this pull request Sep 14, 2024

Add tests to methods that produce links to ensure they still work #3112

Closed

Merge branch 'next' into deprecate/image/avatarLegacy

54fc290

ST-DDT added the s: needs decision Needs team/maintainer decision label Sep 16, 2024

Merge branch 'next' into deprecate/image/avatarLegacy

7a8f45c

Merge branch 'next' into deprecate/image/avatarLegacy

c448372

ST-DDT removed the s: needs decision Needs team/maintainer decision label Sep 19, 2024

ST-DDT requested a review from a team September 19, 2024 15:16

Merge branch 'next' into deprecate/image/avatarLegacy

fedd121

matthewmayer approved these changes Sep 19, 2024

View reviewed changes

import-brain approved these changes Sep 20, 2024

View reviewed changes

ST-DDT merged commit dfbf6c4 into next Sep 20, 2024
21 checks passed

ST-DDT deleted the deprecate/image/avatarLegacy branch September 20, 2024 06:16

Shinigami92 reviewed Sep 20, 2024

View reviewed changes

ST-DDT mentioned this pull request Sep 20, 2024

chore: fix avatarLegacy deprecation version typo #3124

Merged

WontonSam mentioned this pull request Oct 30, 2024

[Snyk] Upgrade @faker-js/faker from 8.3.1 to 9.0.3 WontonSam/aliexpress-product-scraper#20

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(image): deprecate avatarLegacy for removal #3109

refactor(image): deprecate avatarLegacy for removal #3109

ST-DDT commented Sep 14, 2024 •

edited

Loading

netlify bot commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 14, 2024 •

edited

Loading

matthewmayer commented Sep 14, 2024

xDivisionByZerox commented Sep 14, 2024

matthewmayer commented Sep 14, 2024

ST-DDT commented Sep 14, 2024

Shinigami92 commented Sep 14, 2024

ST-DDT commented Sep 14, 2024

Shinigami92 commented Sep 14, 2024 •

edited

Loading

matthewmayer commented Sep 15, 2024

matthewmayer commented Sep 15, 2024

Shinigami92 commented Sep 15, 2024

ST-DDT commented Sep 16, 2024

Shinigami92 commented Sep 16, 2024

ST-DDT commented Sep 16, 2024

Shinigami92 commented Sep 16, 2024

ST-DDT commented Sep 16, 2024

matthewmayer commented Sep 16, 2024

matthewmayer commented Sep 17, 2024

ST-DDT commented Sep 17, 2024

ST-DDT commented Sep 19, 2024

Shinigami92 left a comment

Shinigami92 Sep 20, 2024

ST-DDT Sep 20, 2024

Shinigami92 Sep 21, 2024

ST-DDT Sep 21, 2024 •

edited

Loading

Shinigami92 Sep 21, 2024

refactor(image): deprecate avatarLegacy for removal #3109

refactor(image): deprecate avatarLegacy for removal #3109

Conversation

ST-DDT commented Sep 14, 2024 • edited Loading

netlify bot commented Sep 14, 2024 • edited Loading

✅ Deploy Preview for fakerjs ready!

codecov bot commented Sep 14, 2024 • edited Loading

Codecov Report

matthewmayer commented Sep 14, 2024

xDivisionByZerox commented Sep 14, 2024

matthewmayer commented Sep 14, 2024

ST-DDT commented Sep 14, 2024

Shinigami92 commented Sep 14, 2024

ST-DDT commented Sep 14, 2024

Shinigami92 commented Sep 14, 2024 • edited Loading

matthewmayer commented Sep 15, 2024

matthewmayer commented Sep 15, 2024

Shinigami92 commented Sep 15, 2024

ST-DDT commented Sep 16, 2024

Shinigami92 commented Sep 16, 2024

ST-DDT commented Sep 16, 2024

Shinigami92 commented Sep 16, 2024

ST-DDT commented Sep 16, 2024

matthewmayer commented Sep 16, 2024

matthewmayer commented Sep 17, 2024

ST-DDT commented Sep 17, 2024

ST-DDT commented Sep 19, 2024

Shinigami92 left a comment

Choose a reason for hiding this comment

Shinigami92 Sep 20, 2024

Choose a reason for hiding this comment

ST-DDT Sep 20, 2024

Choose a reason for hiding this comment

Shinigami92 Sep 21, 2024

Choose a reason for hiding this comment

ST-DDT Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

Shinigami92 Sep 21, 2024

Choose a reason for hiding this comment

ST-DDT commented Sep 14, 2024 •

edited

Loading

netlify bot commented Sep 14, 2024 •

edited

Loading

codecov bot commented Sep 14, 2024 •

edited

Loading

Shinigami92 commented Sep 14, 2024 •

edited

Loading

ST-DDT Sep 21, 2024 •

edited

Loading