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

better data sources; fixed measure values; misc #8

Merged
merged 2 commits into from
Nov 15, 2023

Conversation

vird
Copy link
Collaborator

@vird vird commented Nov 14, 2023

No description provided.


// better dev quality, temp solution
async function getMetrics(): Promise<Metrics> {
console.log("DEBUG: getMetrics start");
Copy link
Contributor

Choose a reason for hiding this comment

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

we should consider using logger library (like winston or pino) in order to include debug logs but turn them down in releases

renderer/util/minor.ts Outdated Show resolved Hide resolved
@@ -1,26 +1,23 @@
export type ValueWithMeasureValue = {
Copy link
Contributor

Choose a reason for hiding this comment

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

I love that you made this type. This is a bit controversial in typescript, but you could consider replacing type with interface if it's a simple record type (they are generally faster and more composeable), not a blocker and this is just a comment :)

Comment on lines 53 to 54
const res = await fetch("http://testnet-3.arweave.net:1984/metrics");
const data = await res.text();
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to the PR, but we are perhaps missing try/catch here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For context of this PR no. Because I want that promise should be error, so FE will also receive error, so I can see it in browser console

Copy link
Contributor

Choose a reason for hiding this comment

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

you may end up crashing nodejs if it's not caught anywhere, in a single threaded system it would be fatal error. It's not a big deal for now, I mainly thinking it makes sense to give better error messages. Uncaught errors in promises are quite hard to read and understand.

Copy link
Contributor

@hlolli hlolli left a comment

Choose a reason for hiding this comment

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

I recommend considering the comments, otherwise no blockers

@hlolli hlolli self-requested a review November 15, 2023 11:26
@vird vird merged commit 476ea24 into dashboard-ui Nov 15, 2023
2 checks passed
@vird vird deleted the fix/dashboard-ui branch November 15, 2023 16:36
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.

2 participants