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

feat: add sharing statistics on the homepage #1942

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Jorropo
Copy link

@Jorropo Jorropo commented Jun 6, 2022

Capture d’écran du 2022-06-07 01-18-03

@Jorropo Jorropo force-pushed the share-ratio branch 2 times, most recently from 520a47b to 50f39a9 Compare June 6, 2022 23:19
@Jorropo Jorropo changed the title feat: add share ratio feat: add sharing statistics on the homepage Jun 6, 2022
@Jorropo Jorropo force-pushed the share-ratio branch 4 times, most recently from 8bed475 to c33f0e4 Compare June 7, 2022 16:59
@@ -10,6 +10,7 @@
"countMore": "...and {count} more",
"StatusConnected": {
"repoSize": "Hosting {repoSize} of data",
"shareSize": "Downloaded {downloadedSize} and Shared {sharedSize} ({ratio})",
Copy link
Member

@lidel lidel Jun 24, 2022

Choose a reason for hiding this comment

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

@Jorropo @SgtPooki @hacdias thoughts on presentation / phrasing here?

These are stats for the current session and not the total across history.

Same for "Discovered peers".

Maybe we should prefix them with "Current session" or "Since {startup date}"?

Copy link
Member

Choose a reason for hiding this comment

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

I think we should add some indication that this is not since the beginning, but just for the current session. Maybe some surrounding box saying current session?

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a "Since {date}" prefix.

Also, this is a great candidate for #1220

@lidel lidel force-pushed the main branch 4 times, most recently from dabaee3 to 7ddf870 Compare June 29, 2022 22:19
Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

left a few comments.

@@ -10,6 +10,7 @@
"countMore": "...and {count} more",
"StatusConnected": {
"repoSize": "Hosting {repoSize} of data",
"shareSize": "Downloaded {downloadedSize} and Shared {sharedSize} ({ratio})",
Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of a "Since {date}" prefix.

Also, this is a great candidate for #1220

const bundle = createAsyncResourceBundle({
name: 'bitswapStats',
getPromise: async ({ getIpfs }) => {
const rawData = await getIpfs().stats.bitswap()
Copy link
Member

Choose a reason for hiding this comment

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

What happens if this function throws?

Also, prefer const { dataReceived, dataSent } = await getIpfs().stats.bitswap()

Copy link
Author

Choose a reason for hiding this comment

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

What happens if this function throws?

I have no idea, wtf is javascript anyway ?

(I guess the ressources are never initialised which isn't that bad ?)

Copy link
Member

Choose a reason for hiding this comment

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

updated with try/catch

bundle.selectDownloadedSize = createSelector(
'selectBitswapStats',
(bitswapStats) => {
if (bitswapStats && bitswapStats.downloadedSize) {
Copy link
Member

Choose a reason for hiding this comment

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

eslint should be yelling here about using optional chaining instead, e.g. bitswapStats?.downloadedSize. Also, most of our eslint rules prefer explicit boolean checks, so either

  1. Boolean(bitswapStats?.downloadedSize) or
  2. bitswapStats?.downloadedSize != null

You may want to check your editor settings.

bundle.selectSharedSize = createSelector(
'selectBitswapStats',
(bitswapStats) => {
console.log(bitswapStats)
Copy link
Member

Choose a reason for hiding this comment

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

do we still need this? If so, prefer logger.debug or similar. If not, remove please :)

Copy link
Author

Choose a reason for hiding this comment

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

Debug I forgot to remove

'selectBitswapStats',
(bitswapStats) => {
console.log(bitswapStats)
if (bitswapStats && bitswapStats.sharedSize) {
Copy link
Member

Choose a reason for hiding this comment

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

  1. Boolean(bitswapStats?.sharedSize) or
  2. bitswapStats?.sharedSize != null

src/bundles/bitswap-stats.js Show resolved Hide resolved
const humanRepoSize = humanSize(repoSize || 0)
downloadedSize = downloadedSize || 0
Copy link
Member

Choose a reason for hiding this comment

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

nit: we should prefer the null coalescence operator (??) instead of the OR operator (||) for default values if possible. I assume we also want to make sure downloadedSize is a number? Number(downloadedSize ?? 0)

ditto for other vars here.

@SgtPooki
Copy link
Member

assigning to Dan to push past the finish line

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Prioritized / Ready for Dev
Development

Successfully merging this pull request may close these issues.

6 participants