Skip to content

Port status page script to TypeScript #1570

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

Merged
merged 1 commit into from
Apr 22, 2023
Merged

Port status page script to TypeScript #1570

merged 1 commit into from
Apr 22, 2023

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Apr 18, 2023

For these simple pages, I'm just moving them over to NPM to reduce some code duplication and remove old code. I'm not trying to refactor them heavily nor describe the server data response in super detail (it would be quite annoying for all the enum variants). The biggest benefit of TS should be in the pages that use Vue (the compare page and soon the runtime UI page(s)).

For the status page, the downloaded JS bundle size goes down from ~15k to ~3k (not that it's super important here).

@Kobzol Kobzol requested a review from rylev April 18, 2023 20:22
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

Seems ok to me. My only comment is that there are two uses of any. Can they be avoided? If not, brief comments explaining why (and what form the data takes) would be helpful.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 19, 2023

Yeah so this relates to this comment:

nor describe the server data response in super detail (it would be quite annoying for all the enum variants).

The Rust side uses enums which are encoded by serde in a way that is not super TypeScript friendly. I could model them (or change the serde encoding), but I'm not sure if it provides a lot of value, since it does not give us any guarantee that the incoming data will have this specific shape anyway - everytime the server side changes, we need to update the TS side too. And since this data response is only used on one place in this file, I decided to only model the easy stuff and leave the rest as any.

@Kobzol
Copy link
Contributor Author

Kobzol commented Apr 22, 2023

Added a comment explaining the usage of any.

@Kobzol Kobzol enabled auto-merge April 22, 2023 17:51
@Kobzol Kobzol merged commit 0496037 into master Apr 22, 2023
@Kobzol Kobzol deleted the npm-status branch April 22, 2023 18:04
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