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

[vtadmin-web] Query vtadmin-api from vtadmin-web with fetch + react-query #7239

Merged

Conversation

doeg
Copy link
Contributor

@doeg doeg commented Jan 3, 2021

Description

tl;dr:
This PR implements the humble beginnings of our vtadmin-api HTTP fetchers using native fetch and react-query. The API boundary is one of the spicier parts of any client, particularly when it comes to testing and (strong) typing. To that end, I focused on adding a bunch of tests + commentary -- I would love your feedback on these parts, in particular, if you have any.

Now the usual rambling...

react-query is still somewhat new to me, and Mock Service Workers are brand new to me. I really like how this implementation came together, but I expect a lot of unknown unknowns (for me) here. So I've tried to keep this implementation very modular + flexible in anticipation of us wanting to, say, swap in redux instead of react-query, or use gRPC instead of HTTP.

I expect the functions in api/http.ts to become more general as we add more bindings for other vtadmin-api endpoints. I also have a feeling we'll want to pull out the Mock Service Worker parts of api/http.test.ts (especially since we could re-use it as a hermetic/offline development server, which might be interesting). For this PR, though, I tried to keep things straightforward and co-located.

This is also the first use of one (1) environment variable!

It still doesn't look like much... but! The exciting thing is that vtadmin-web is now issuing HTTP requests against vtadmin-api and populating the UI with ✨ real data ✨ from the local Vitess + Docker example.

Screen Shot 2021-01-03 at 2 25 23 PM

A couple of TODOs for me that I'll need to ticket:

  • Add CI targets for vtadmin-web unit tests + linting
  • Streamline the local development process described in the README

Related Issue(s)

Checklist

  • Should this PR be backported?
  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

N/A (yet)

Impacted Areas in Vitess

Components that this PR will affect:

  • Query Serving
  • VReplication
  • Cluster Management
  • Build

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg doeg requested a review from ajm188 as a code owner January 3, 2021 19:43
Copy link
Contributor

@ajm188 ajm188 left a comment

Choose a reason for hiding this comment

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

I really really like this setup. I have one concern about error testing, but after that this lg2m

"host": {
"hostname": "127.0.0.1:15992"
},
"tags": ["dead-dove-do-not-eat"]
Copy link
Contributor

Choose a reason for hiding this comment

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

but where did the lighter fluid come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(muffled final_countdown.wav plays in the distance)

// means our fake is more robust than it would be otherwise. Since we are using
// the exact same protos in our fake as in our real vtadmin-api server, we're guaranteed
// to have type parity.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

(curious) As a Go programmer, this trailing comment line looks odd to me. Is this JS/TS convention that I should learn to get used to?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah I just like whitespace for multi-paragraph comments. I can remove it though!

Copy link
Contributor

Choose a reason for hiding this comment

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

No, totally fine!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Too bad I did it anyway!!! 😈

ok: false;
}

type HttpResponse = HttpOkResponse | HttpErrorResponse;
Copy link
Contributor

Choose a reason for hiding this comment

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

God I love union types so much, go2 pls 🥺

await api.vtfetch(endpoint);
} catch (e) {
/* eslint-disable jest/no-conditional-expect */
expect(e.message).toEqual('invalid http envelope');
Copy link
Contributor

Choose a reason for hiding this comment

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

One of the potential downsides to doing exact equality checks on error messages means that if we ever add additional context to errors, then we can potentially break a bunch of tests for an unrelated change. Do we want to force that, or does it make sense to expect().contains()?

Copy link
Contributor Author

@doeg doeg Jan 3, 2021

Choose a reason for hiding this comment

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

Hm, probably the best way is to extend Error with custom subclasses. Unfortunately, this is pretty cumbersome to do in JS/TS (especially compared to go 🥺) BUT we can then do assertions like expect(e.name).toEqual('InvalidHttpEnvelope') or whatever, which also gives us more flexibility with e.message.

I considered this before and didn't do it because I'm uh... well, it's a little bit of work 😬 but it's super worth it, so thanks for pointing this out!

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@doeg
Copy link
Contributor Author

doeg commented Jan 3, 2021

@ajm188 thank you for the great suggestions. 🙏 I made a couple of changes:

  • I defined subclasses of Error for anticipated failure scenarios in api/http.ts per your comment here. The tests are a lot sturdier now.

  • I added a test case for when /api/tablets returns an { ok: false } response.

  • I added a dumb error placeholder to the UI, which looks silly but shows off how nice react-query's error handling is.

Screen Shot 2021-01-03 at 5 29 43 PM

Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
@deepthi deepthi merged commit 446725d into vitessio:master Jan 5, 2021
@askdba askdba added this to the v9.0 milestone Jan 6, 2021
@doeg doeg added this to In progress in VTAdmin via automation Mar 16, 2021
@doeg doeg moved this from In progress to Done in VTAdmin Mar 16, 2021
@doeg doeg added the Component: VTAdmin VTadmin interface label Mar 16, 2021
@doeg doeg changed the title [vtadmin] Query vtadmin-api from vtadmin-web with fetch + react-query [vtadmin-web] Query vtadmin-api from vtadmin-web with fetch + react-query Mar 16, 2021
@doeg doeg deleted the sarabee-vtadmin-trying-to-make-fetch-happen branch November 1, 2021 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: VTAdmin VTadmin interface
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants