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

enhance: アバターをサーバーで圧縮して取得する #8337

Closed
wants to merge 46 commits into from

Conversation

tamaina
Copy link
Contributor

@tamaina tamaina commented Feb 20, 2022

A part of #8319

#8216 のつづき

What

  • /avatar.webpにクエリ付きでアクセスすると、256x256にクロップ(cover)したアバターwebpを取得する or identiconへリダイレクト
    • クライアント各所でこのURLを使うように
  • /avatarは削除

Why

通信量削減

@tamaina tamaina changed the title アバターはサーバーで圧縮して取得する enhance: アバターをサーバーで圧縮して取得する Feb 20, 2022
@tamaina
Copy link
Contributor Author

tamaina commented Feb 20, 2022

言うまでもなくp1.a9z.devで動作中

Comment on lines -188 to +195
return `${config.url}/identicon/${user.id}`;
return this.getIdenticonUrl(user);
}
}

public getIdenticonUrl(user: User): string {
return `${config.url}/identicon/${user.id}`;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why moving the same code into a separate function is necessary? Since its only one line, at most I think a comment would make sense here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used at

return ctx.redirect(Users.getIdenticonUrl(user));

//#endregion

ctx.set('Content-Type', 'image/webp');
ctx.set('Cache-Control', 'max-age=31536000, immutable');
Copy link
Contributor

@Johann150 Johann150 Feb 20, 2022

Choose a reason for hiding this comment

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

Since avatars can change multiple times in the span of a year I think this caching is a bit too agressive. I think something between a week and a month might make more sense? Or remove immutable because that is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ここに至るまでに、クエリ文字列にurlを持たない場合はリダイレクトされるはずなのですが、どうでしょうか。

@tamaina
Copy link
Contributor Author

tamaina commented Feb 21, 2022

アバターって本来thumbnailが挿入されるべきなんだけど、p1.a9z.dev(TLに出現するのがほとんどリモートユーザー)でwebpublicばっかりになっているのは #7767 のバグのせいだということを理解した…

@tamaina tamaina closed this Feb 21, 2022
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.

3 participants