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: make api's for graphs we use instead of templating json #277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KolbyML
Copy link
Member

@KolbyML KolbyML commented May 29, 2024

Update index.html to fetch data from api's instead of embedding json. Embedding data makes caching the page not possible so it has to be fully loaded every time.

@KolbyML KolbyML requested a review from mrferris May 29, 2024 10:06
@KolbyML KolbyML self-assigned this May 29, 2024
@KolbyML KolbyML requested a review from pipermerriam May 29, 2024 10:06
@mrferris
Copy link
Collaborator

I think that the result of the discussion was that we wanted either/both APIs for getting just important values (eg number of nodes seen in the last 24 hours), or fully embeddable HTML elements. I think that returning an entire JSON object to the ethportal website leaves way more front-end work/maintainence on the ethportal website than we'll want.

@KolbyML
Copy link
Member Author

KolbyML commented Jun 5, 2024

I think that returning an entire JSON object to the ethportal website leaves way more front-end work/maintainence on the ethportal website than we'll want.

I don't think this is how it works. This PR doesn't create any additional maintenance work, it actually creates less. Currently the website is built as if it was 2012. This PR better aligns the project with modern web practices.

I think that returning an entire JSON object

We are currently doing this already. So I am not sure what is being referred to here. before this PR we are embedding json into a webpage which is extreme malpractice. Doing this causes our html pages unable to be cached by CDN's. The difference here is instead of embedding json (which is very bad), we are fetching json which is proper practice.

I could go further into how this helps productivity, but I believe the issues I brought to light above would have to be addressed before that kind of discussion would be productive.

fully embeddable HTML elements

This is a completely separate issue and is not directly related to this PR..

@mrferris
Copy link
Collaborator

mrferris commented Jun 5, 2024

There's been a misunderstanding regarding the conversation that was had at the summit.

We were not talking about having glados call JSON endpoints like you're doing here (I agree that loading HTML/JS that itself fetches JSON is better, which is why the stats history and census explorer use that approach, but calling HTML templating "malpractice" is a bit much, and certainly not an issue that would've been worth discussing at summit).

We were talking about giving ethportal.net (a separate website from glados) access to some of the components or data within glados for marketing purposes. And having ethportal.net load a big JSON object containing all of the census data and then do its own processing to obtain simple metrics like "how many live nodes on the portal network?" is outside the scope of ethportal.net. It'd be preferable to instead have a simple endpoint that returns a single number that ethportal.net can hit and not have to do any processing on. ethportal.net's processing shouldn't have to change if the data format going into glados's front end changes, they should be decoupled.

That said, if you add a PR description this can move forward. But these endpoints are not relevant to any discussion that took place at summit.

@KolbyML
Copy link
Member Author

KolbyML commented Jun 5, 2024

@mrferris sounds good I updated the descriptions

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants