Skip to content
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

Catch errors when uploading white avatar #3119

Merged
merged 1 commit into from
Oct 25, 2021

Conversation

askvortsov1
Copy link
Sponsor Member

Fixes #2714

Changes proposed in this pull request:
Catch errors in color thief; if the error corresponds to the one thrown when completely white avatars are uploaded, silence the error and directly set the avatarColor value.

Reviewers should focus on:
Unfortunately JS doesn't really provide us with a cleaner way to catch only this error. I'm hesitant to check for the contents of the error message since it's quite generic.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

Copy link
Member

@clarkwinkelmann clarkwinkelmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not tested, but looks good to me.

I'm not even sure the instanceof check is required. We could silence all errors that result of calling ColorThief.

Or if we wanted to not catch errors that result from user.avatarColor = (if an extension defines a setter function), we could save the color in a variable and then set the user avatar color outside of the try/catch.

@matteocontrini
Copy link
Contributor

(Shouldn't the title contain "catch" instead of "escape"? 🤔)

@dsevillamartin
Copy link
Member

We could silence all errors that result of calling ColorThief.

I'd rather not do this. Catching all TypeErrors is already too much, but not much we can do about it (error message changes depending on minification and other things sometimes)

@askvortsov1 askvortsov1 changed the title Escape errors when uploading white avatar Catch errors when uploading white avatar Oct 25, 2021
@askvortsov1
Copy link
Sponsor Member Author

(Shouldn't the title contain "catch" instead of "escape"? thinking)

Nice, uh, catch 🙈

We could silence all errors that result of calling ColorThief.

I'd rather not do this. Catching all TypeErrors is already too much, but not much we can do about it (error message changes depending on minification and other things sometimes)

Are you happy with this implementation then?

@dsevillamartin
Copy link
Member

Are you happy with this implementation then?

I don't love it, but I think it's the best we can do without forking color thief.

@askvortsov1 askvortsov1 merged commit a661376 into master Oct 25, 2021
@askvortsov1 askvortsov1 deleted the as/color-thief-white-avatar-workaround branch October 25, 2021 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

White avatar image throws javascript errors on profile page
4 participants