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

refactor: 可読性のため一部でArray.prototype.atを使うように #11274

Merged
merged 2 commits into from
Jul 14, 2023
Merged

refactor: 可読性のため一部でArray.prototype.atを使うように #11274

merged 2 commits into from
Jul 14, 2023

Conversation

okayurisotto
Copy link
Contributor

What

items[items.length - 1]のようにしている部分をitems.at(-1)のように書き換えました。

Why

可読性をより良くするため。

Additional info (optional)

型がT[]である配列に対してArray.prototype.at()を実行すると、返り値の型はT | undefinedになります。これまでのブラケット記法でのアクセスでは常にTが返されていたため1、このPRではundefinedでないかどうかの確認を行う処理も、簡単に追加できそうな部分に関しては追加しています。

ただ簡単には追加できそうにない部分もあったため、そちらでは暫定的にnon-null assertionを使ってしまっています。追加しようにも変更が大きくなってしまいそうで、このPRにすべての変更を含めてしまうのはよくないのではないかというように判断しました。

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

Footnotes

  1. https://www.typescriptlang.org/tsconfig#noUncheckedIndexedAccess が有効になっていないため

@github-actions github-actions bot added packages/frontend Client side specific issue/PR packages/backend Server side specific issue/PR packages/backend:test labels Jul 13, 2023
@github-actions github-actions bot requested review from syuilo and tamaina July 13, 2023 22:20
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #11274 (3ed923d) into develop (af30959) will increase coverage by 0.08%.
The diff coverage is 20.83%.

@@             Coverage Diff             @@
##           develop   #11274      +/-   ##
===========================================
+ Coverage    77.64%   77.72%   +0.08%     
===========================================
  Files          908      739     -169     
  Lines        91878    70787   -21091     
  Branches      7670     7233     -437     
===========================================
- Hits         71335    55018   -16317     
+ Misses       20543    15769    -4774     
Impacted Files Coverage Δ
packages/backend/src/misc/prelude/array.ts 61.15% <0.00%> (-0.45%) ⬇️
.../queue/processors/DeleteAccountProcessorService.ts 41.66% <0.00%> (ø)
...ueue/processors/ExportFavoritesProcessorService.ts 28.75% <0.00%> (ø)
...ueue/processors/ExportFollowingProcessorService.ts 36.13% <0.00%> (ø)
...rc/queue/processors/ExportNotesProcessorService.ts 30.06% <0.00%> (ø)
...ges/backend/src/server/ActivityPubServerService.ts 53.95% <0.00%> (ø)
...eue/processors/CleanRemoteFilesProcessorService.ts 42.85% <33.33%> (ø)
...eue/processors/DeleteDriveFilesProcessorService.ts 43.58% <33.33%> (ø)
...queue/processors/ExportBlockingProcessorService.ts 33.91% <33.33%> (ø)
...c/queue/processors/ExportMutingProcessorService.ts 35.29% <33.33%> (ø)
... and 1 more

... and 169 files with indirect coverage changes

@syuilo syuilo merged commit 2b6dbd4 into misskey-dev:develop Jul 14, 2023
17 checks passed
@syuilo
Copy link
Member

syuilo commented Jul 14, 2023

👍

@acid-chicken
Copy link
Member

もうマージされちゃったけど Array#at って現実装下の一部ではパフォーマンス上の問題なかったっけ

@okayurisotto okayurisotto deleted the array-prototype-at branch July 14, 2023 08:58
@okayurisotto
Copy link
Contributor Author

指摘を受けて調べてみたところ、たしかにそのような話がある(あった)ようでした。

ただ実際に自分でも簡単なベンチマークを実行してみたところ、少なくとも現在の最新バージョンのMisskey(13.14.0-beta.2)が使うNode.jsであるv20.3.1では、パフォーマンス上の問題は見受けられませんでした。

現在LTSであるv18.16.1ではたしかにパフォーマンスに問題があるように見えるため、その間にNode.js(もしくはV8?)の実装がよりよくなったのだと思います。

LTSでパフォーマンス問題が起きてしまっているのは少し残念ですが、Misskeyが公式にサポートしているバージョンではないため問題はない、ということで構いませんか?

@acid-chicken
Copy link
Member

Misskeyが公式にサポートしているバージョンではない

そうなのかしら

@syuilo
Copy link
Member

syuilo commented Jul 14, 2023

公式にサポートしてるバージョンではあるけど推奨バージョンではない

@okayurisotto
Copy link
Contributor Author

すみません、間違えました。CHANGELOGには「v13.12.0からのMisskeyはv18.6.0以上のNode.jsが必要」としか書かれていないため、現在のLTSであるv18.16.1はサポートされている環境です。

私が「サポートされているNode.jsのバージョン」と誤認してしまったのは、.node_versionで指定されたバージョンの方です。そちらではv20.3.1を使用するように指定されていて、開発時に使われることが想定されているほか、各種テストの実行でもそのファイルが参照されています。また、DockerfileでもそのバージョンのNode.jsを使用するように書かれています。テストが行われていないバージョンでも動かされることを想定しているというのをド忘れしてしまっていました。

話を脱線させてしまうようで申し訳ないのですが、「サポートしているものの推奨はしておらず、それゆえテストの実行で使われていないバージョンのNode.js」にはまだない機能を開発時に誤って使用してしまったとき、そのミスを知ることができる仕組みはありますか? 今回のArray.prototype.atは(使えないわけではないためそういう意味では)大丈夫でしたが、今後何かの拍子に使用してしまうことがあるかもしれません。tsconfig.jsonが適切に設定されていた場合にはこのミスを見付けられるかもしれませんが、@tsconfig/node18とMisskeyがbackendで使用しているtsconfig.jsonではlibの設定に相違があります(esnextが許容されている)。

あとfrontendの存在をすっかり忘れてしまっていました。frontendでどのようなpolyfillが挿入されているのかまだ理解できていないのですが、やはりvite.config.tsで指定されている環境でどの程度のパフォーマンスの違いが発生するのか、実際にベンチマークを取るべきなのでしょうか。

Misskey開発でこの辺りがどのようになっているのか詳しくないので申し訳ないです。

slofp pushed a commit to Secineralyr/misskey.dream that referenced this pull request Jul 21, 2023
* refactor: `Array.prototype.at`を使うように

* fixup! refactor: `Array.prototype.at`を使うように
u1-liquid pushed a commit to u1-liquid/misskey that referenced this pull request Jul 24, 2023
riku6460 pushed a commit to MisskeyIO/misskey that referenced this pull request Jul 25, 2023
* Revert "refactor: 可読性のため一部で`Array.prototype.at`を使うように (misskey-dev#11274)"

This reverts commit 2b6dbd4.

* Apply suggestions from code review

---------

Co-authored-by: okayurisotto <okayurisotto@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend:test packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants