refactor: reorganize API logic and create class/hook for simplifying proxy logic #124

Parkreiner · 2024-04-26T18:55:22Z

Think I finally have things figured out. This is part 1 of a new set of PRs for bringing the Coder SDK into Backstage.
Connected to #107

Changes made

I'd say most of these changes qualify as me cleaning up previous messes, and making it easier to build on the plugin going forward.

Split the previous api.ts file up:
- /api/api.ts - Contains the majority of the previous fetch logic. This will eventually become CoderClient
- /api/errors.ts - Contains custom error definitions
- /api/queryOptions.ts - Contains the query option factories that should be used in multiple places throughout the codebase
- Almost all previous auth logic has been consolidated into CoderAuthProvider.tsx
Created a UrlSync class for managing URLs a lot more consistently
- It's basically a wrapper over Backstage's DiscoveryApi, and is designed to work with useSyncExternalStore
- The idea is that when CoderClient needs to make a request, it will get it through UrlSync. UrlSync will then detect when the API endpoints change and notify React components through the hook
  - This is almost exactly the same setup that Backstage recommends through DiscoveryApi, except that we have better React UI support
Upgraded the useBackstageEndpoints hook into useUrlSync
Modified the workspaces queries so that they'll periodically refetch if any of the workspaces returned were in a pending state
Updated the mock server middleware to ensure that all requests have a valid Coder session token

Notes

@code-asher I know we talked about how trying to fix the DiscoveryApi jank right now wasn't worth it, but the more I thought about it, the more I realized that it would fix a lot of problems
- Help centralize a bunch of ad-hoc API logic I have strewn all over the codebase, and make sure that they can't get out of sync with each other
- Make it a lot easier to implement CoderClient – a lot of the properties I was originally trying to expose through it were better suited for UrlSync
I also approached this PR more incrementally, to ensure I wasn't breaking tests left and right
- One of the problems with the previous PRs is that I still don't know how to get the tests 100% passing without trying to cheat the tests. But if we're cheating the tests, that's just going to destroy our trust in them
Next PRs will cover the CoderClient, and then the Coder SDK
- I'm going to wait until the dust settles a bit before trying to make a universal auth system for token auth/OAuth

Parkreiner · 2024-04-26T19:14:36Z

plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx

+  const queryIsEnabled = authToken !== '';
+  const authValidityQuery = useQuery<boolean>({
+    queryKey: [...sharedAuthQueryKey, authToken],
+    enabled: queryIsEnabled,
+    keepPreviousData: queryIsEnabled,
    refetchOnWindowFocus: query => query.state.data !== false,
+    queryFn: async () => {
+      // In this case, the request doesn't actually matter. Just need to make any
+      // kind of dummy request to validate the auth
+      const requestInit = await getCoderApiRequestInit(authToken, identityApi);
+      const apiEndpoint = await urlSyncApi.getApiEndpoint();
+      const response = await fetch(`${apiEndpoint}/users/me`, requestInit);
+
+      if (response.status >= 400 && response.status !== 401) {
+        throw new BackstageHttpError('Failed to complete request', response);
+      }
+
+      return response.status !== 401;
+    },


Moved the auth query options factory function here to consolidate the logic more (especially since this is the only file that needs it), and then, to make setting up the function itself easier, I inlined everything

The logic here gets simplified down a lot more with the CoderClient PR, where the entire value of queryFn becomes () => coderClient.syncToken(authToken)

Parkreiner · 2024-04-26T19:36:44Z

plugins/backstage-plugin-coder/src/api/api.ts

+type TempPublicUrlSyncApi = Readonly<{
+  getApiEndpoint: UrlSync['getApiEndpoint'];
+  getAssetsEndpoint: UrlSync['getAssetsEndpoint'];
+}>;


Hoping to get rid of this in the next PR. I just couldn't without making the CoderClient work start bleeding into this PR

Removed as of the next PR

Parkreiner · 2024-04-26T19:55:56Z

plugins/backstage-plugin-coder/src/hooks/useUrlSync.ts

+  /**
+   * @todo This is a temporary property that is being used until the
+   * CoderClientApi is created, and can consume the UrlSync class directly.
+   *
+   * Delete this entire property once the new class is ready.
+   */
+  api: Readonly<{
+    getApiEndpoint: UrlSync['getApiEndpoint'];
+    getAssetsEndpoint: UrlSync['getAssetsEndpoint'];
+  }>;


Again, hoping to get rid of this with the next PR

Also now removed via the new CoderClient PR

code-asher · 2024-04-29T19:06:16Z

plugins/backstage-plugin-coder/src/api/UrlSync.ts

+    getCachedUrls: () => UrlSyncSnapshot;
+  }>;
+
+export class UrlSync implements UrlSyncApi {


This is sync as in synchronous, right, because you can access the URLs synchronously? Not worth blocking over the name, but it does seem at odds with how there are async methods on the class, which look like the main usages of the class except for isEmojiUrl which uses assetsRoute.

Just about to log off, so I won't be looking at the rest of the comments until tomorrow morning, but really quick: it's actually "sync" as in "synchronization"

The main goal of the class is synchronizing React with the Backstage APIs (and also working with useSyncExternalStore, which also uses "sync" in the "synchronizing" sense)

Could probably rename things to remove that ambiguity

Ahhhhhhhh I probably had the ...Sync methods in Node's fs on my brain. No really good alternatives come to my mind, so I am not opposed to just leaving it.

code-asher · 2024-04-29T19:11:58Z

plugins/backstage-plugin-coder/src/api/UrlSync.ts

+  // ConfigApi is literally only used because it offers a synchronous way to
+  // get an initial URL to use from inside the constructor. Should not be used
+  // beyond initial constructor call
+  private readonly configApi: ConfigApi;


My instinct is that if we only need one synchronous property off configApi in the constructor, we might as well make that clear in the parameters as well, and just pass in a string for the base URL rather than the entire API (easier to mock too, I guess). Unless we might need something else off it later. Not a blocker, passing the entire thing does make it easier to use too, so maybe not worth changing.

Yeah, I see what you mean. My main worry is that from all the Backstage examples I've seen, they've received the entire API directly, so there might be value in keeping consistency with that, especially if we start getting open-source contributions

I also feel like if a user of the API is required to pass in only a string, they're more at risk of passing in the wrong one. If we defer that logic to Backstage, and keep things centralized in UrlSync, that helps improve the odds that the URLs will never be wrong. If they are, that'll be a bug with Backstage's main code

Luckily, the entire ConfigApi is basically a one-method interface, so it won't ever be hard to mock out, but one thing I can do in the meantime is split the difference. As in, receive the configApi value from the constructor inputs, but only use it for the constructor call, and never embed it

code-asher · 2024-04-29T19:25:51Z

plugins/backstage-plugin-coder/src/api/UrlSync.ts

+    const proxyRoot = await this.discoveryApi.getBaseUrl(
+      PROXY_URL_KEY_FOR_DISCOVERY_API,
+    );


Tangential to this PR, but I still think we should consider making queries directly to the Coder API and remove the proxy altogether.

Backstage docs do say this makes senses under circumstances that appear to apply to us, and the reasons for doing it via a proxy do not seem relevant to us: https://backstage.io/docs/plugins/call-existing-api#issuing-requests-directly

But, the reason I bring it up now is that it would eliminate the need for both configApi and discoveryApi, I think, at least as they are used here, so it would affect this new class. We only need the URL on the Coder config object, if I recall correctly.

Yeah, that you mention it, as long as we know the single endpoint, and since we're in control of how it's structured, I don't think there would be any issues with ditching the proxy. It would also remove the need for the user to update their app-config.yaml file

I'm going to keep the code the same as-is for now, just because there's two other PRs depending on it, and because even if this isn't the final iteration, the API code does get consolidated a lot more. The current code should make it easier to remove the proxy and extra APIs down the line

Makes sense to me!

code-asher · 2024-04-29T19:48:19Z

plugins/backstage-plugin-coder/src/api/UrlSync.ts

+    );
+
+    const newSnapshot = this.prepareNewSnapshot(proxyRoot);
+    this.urlCache.updateSnapshot(newSnapshot);


If you have to call these methods to update the snapshot, you would always need to call these methods, right? Which means you should already have the endpoint you need, and have no need for state.

If we had something like discoveryApi.subscribe(() => urlCache.update()), then it would make more sense to me, because then we would not need the methods at all, the snapshot would always be up to date, and we could just access it synchronously off useUrlSync().state (no idea if discoveryApi has that, just thinking out loud).

I see the only place we access the synchronous state is isEmojiUrl which uses assetsRoute, but IMO we could just remove that function and remove the border radius. The Coder dashboard does not use any rounding, which I think makes sense; even some of the non-emoji icons we use for templates are meant to be square or have tips that get cut off.

tl;dr maybe we do not need any snapshot in this class if we get rid of the one current usage? Although, then it becomes a very thin wrapper around configApi and discoveryApi, and maybe it will make sense to call them directly.

Yeah, a subscription would be nice, and I think that would ultimately be the best design – I'm just not completely sure what the best way to implement it with the current APIs would be. DiscoveryApi doesn't have a subscription method

As far as the rounding, though, that was basically an aesthetic decision. If I look at my Coder dashboard right now:

I think the images look a little sloppy, because there's no "aesthetic consistency" between them. They're not using the same colors, and by default, don't have the same shape, so the only thing giving them visual rhythm is the fact that they're placed consistently in the same spot. Enforcing rounding increases their similarities, and makes them look like they're supposed to be similar. I actually think removing the rounding would make make the UI look less polished

I've actually been meaning to open a PR for Coder core to update the CSS to get the emoji rounding issues fixed, too

It might make sense to leave being consistent up to the folks uploading the icons, as frustrating as that is. If you check out the templates page, some of the non-emoji icons are non-circle shapes which I think look weird or wrong when they are forcefully rounded.

I think this is why we have the padding to emojis, to avoid cutting them off, but regular images can have the same problem. How do we tell the difference between an emoji or emoji-like image and one that can be rounded?

Some would look better rounded though, maybe it makes sense to add a "Round this icon?" option to the template settings, otherwise folks have to manually round their icons which seems annoying. Although, it seems like this mostly only happens when the image is kinda bad to begin with, and includes a background color. And sometimes they contain extra space making them overly small which I guess there is not much we can do about except ask the user if they want to cut off some of the image or something.

Also sometimes it seems we display a square person icon (on the dashboard, not sure what would happen in Backstage), which I think happens when the image does not load? Seems like we should put something else there.

Yeah, good point – I could go either way, honestly

I feel like user avatars (and other images that are basically for UI purposes) kind of have the expectation that they can be morphed or changed into any number of shapes, and that if the image the user supplies doesn't look good with the styling the vendor/app uses, it's the user's responsibility to fix that. I can't remember the last time I've seen a modern site not use some amount of rounding with their "UI-ish" images. But I don't think workspaces/templates quite have that expectation

With the emoji, we at least know that they will always come from the same source (I don't think we give users the ability to define custom emoji), so they're all working with the same uniform constraints and can be offset the same consistent way

Realistically, though, most icons are going to have their main content in the center, so they naturally lend themselves to being rounded. I think I'd rather choose making 95% of icons look better, with 5% looking worse, over 100% looking "okay-ish". I'm not sure if this is worth exposing another property to the end user, though

And the plugin does have logic to handle there not being any images, too – those use the same rounding

If we get rid of the rounding for the images, I'm not sure what to do with the fallbacks. I guess we could keep those rounded, but jumping from non-rounded to rounded might be weird. And even if we don't completely remove the rounding, and just reduce it, I think the UI still starts to look a little funny

I feel like user avatars (and other images that are basically for UI purposes) kind of have the expectation that they can be morphed or changed into any number of shapes [...] But I don't think workspaces/templates quite have that expectation

I think that expectation can make sense for images, but not as much for icons. We use mostly emojis and logos for icons, but I wonder if that is the case generally for other users.

Tangentially, the current devcontainers icon feels odd to me because it has some built-in padding making it look too small in comparison. I tried a different icon without the padding so it matches the other icons, what do you think?

With the emoji, we at least know that they will always come from the same source (I don't think we give users the ability to define custom emoji), so they're all working with the same uniform constraints and can be offset the same consistent way

A custom emoji could be added like any other icon but yeah no way to modify the default set of emojis pretty sure, unless maybe you build from source with your own emojis.

From playing around on the dashboard, we would need 5px padding to prevent emojis from getting cropped, and that makes them look unusually small compared to the other icons. Is it just me though?

Realistically, though, most icons are going to have their main content in the center, so they naturally lend themselves to being rounded. I think I'd rather choose making 95% of icons look better, with 5% looking worse, over 100% looking "okay-ish".

With the bundled icons and the ones we have added at least, we might make more look worse than better (going off the templates page and https://dev.coder.com/icons), but that might not hold true for icons added by other users.

I think it is not so much about being centered (which I think all ours are), but about being able to fit in the bounds of a circle. For example the Vault triangle turns into a slice of pizza, JFrog on the templates page becomes IFrog with a mangled g, Kotlin turns into Pac-Man 😂 and Fedora loses its little tail.

And the plugin does have logic to handle there not being any images, too – those use the same rounding

Ah I was thinking about the case where there is an image, but it is a 404.

And even if we don't completely remove the rounding, and just reduce it, I think the UI still starts to look a little funny

I actually quite like the decreased rounding on those fallbacks!! But I also like Lisp so who knows if I can be trusted. The letters alone without a background, maybe extra bold, could look cool too. Good topic for FE variety maybe?

I probably sound really passionate about this (maybe I am lol) but I mostly feel motivated to remove it because it means the entire snapshot and subscription code in this class goes away. I think it felt like a lot to me for all that code only to support rounding! 😛

To add one more argument to the small book I have written here, we display these icons in Backstage, the dashboard, and the Gateway plugin; right now Backstage is the odd one out.

code-asher · 2024-04-29T20:15:23Z

plugins/backstage-plugin-coder/src/api/queryOptions.ts

+// Defined here and not in CoderAuthProvider.ts to avoid circular dependency
+// issues


🎶 this is the song that never ends 🎶

code-asher · 2024-04-29T20:17:35Z

plugins/backstage-plugin-coder/src/api/queryOptions.ts

+  // Disabling query object when there is no query text for performance reasons;
+  // searching through every workspace with an empty string can be incredibly
+  // slow.
+  const enabled = inputs.auth.isAuthenticated && inputs.coderQuery !== '';


Tangential to the PR since it has been like this for a while, but should we do:

Suggested change

const enabled = inputs.auth.isAuthenticated && inputs.coderQuery !== '';

const enabled = inputs.auth.isAuthenticated && inputs.coderQuery.trim() !== '';

Then again, I have been using a space to query all workspaces for testing so I kinda like that I can do that.

On the flip side, maybe we should mimic the dashboard behavior which allows blank input. Maybe not until after we use the new parameters endpoint though, so we are not blasting off hundreds of queries. 😆

Oh, good call – I can add trim really quickly

code-asher · 2024-04-29T20:22:23Z

plugins/backstage-plugin-coder/src/components/CoderProvider/CoderAuthProvider.tsx

+        throw new BackstageHttpError('Failed to complete request', response);
+      }
+
+      return response.status !== 401;


This boolean indicates whether the token is valid, right?

Maybe safer to explicitly check for 20x rather than not 401 (I know we return on >=400 above, but not sure if we can get 30x codes, maybe if there is some kind of infinite redirect misconfiguration, and maybe there are very weird cases where a misconfiguration returns a 10x for some reason, and who knows what other codes might be added in the future).

That the response includes a user, to make sure we really did hit the deployment and not just some other thing that returns a 20x (for example suppose there is temporarily an HTML page here that returns 200 because the deployment is down for maintenance).

Not a blocker though since this is the status quo!

Yeah, good call – I think it'll be easier to make this change for the CoderClient PR, so I'll make the changes there

code-asher

Whoops, meant to approve. Looks good to me, but I do think UrlSync might not be necessary depending on what we decide about the proxy.

Parkreiner added 25 commits April 26, 2024 13:43

wip: commit progress on UrlSync class/hook

67cb4b3

refactor: consolidate emoji-testing logic

e53d127

docs: update comments for clarity

f26184f

refactor: rename helpers to renderHelpers

b54324b

wip: finish initial implementation of UrlSync

7daf597

chore: finish tests for UrlSync class

86b5acc

chore: add mock DiscoveryApi helper

5d14d5a

chore: finish tests for useUrlSync

afd0203

refactor: consolidate mock URL logic for useUrlSync

1e16c82

fix: update test helper to use API list

d786e34

fix: remove unneeded imports

063ecf2

fix: get tests for all current code passing

1d1ab3c

fix: remove typo

58566c8

fix: update useUrlSync to expose underlying api

cd5fef6

refactor: increase data hiding for hook

abfd949

fix: make useUrlSync tests less dependent on implementation details

26ae96d

refactor: remove reliance on baseUrl argument for fetch calls

5aabe86

refactor: split Backstage error type into separate file

425d50f

refactor: clean up imports for api file

04f0f3e

refactor: split main query options into separate file

111df43

consolidate how mock endpoints are defined

1575f8e

fix: remove base URL from auth calls

dc6e75c

refactor: consolidate almost all auth logic into CoderAuthProvider

fd8b3cb

move api file into api directory

5af006c

fix: revert prop that was changed for debugging

644e632

Parkreiner self-assigned this Apr 26, 2024

fix: revert prop definition

a667b48

Parkreiner changed the title ~~refactor: reorganize API logic~~ refactor: reorganize API logic and create class/hook for simplifying proxy logic Apr 26, 2024

refactor: extract token-checking logic into middleware for server

cca343d

Parkreiner requested a review from code-asher April 26, 2024 19:09

Parkreiner commented Apr 26, 2024

View reviewed changes

refactor: move shared auth key to queryOptions file

aac5a0c

Parkreiner commented Apr 26, 2024

View reviewed changes

docs: add reminder about arrow functions

32fb344

code-asher reviewed Apr 29, 2024

View reviewed changes

code-asher approved these changes Apr 29, 2024

View reviewed changes

Parkreiner mentioned this pull request Apr 29, 2024

refactor(Coder plugin): update workspace queries to use updated API endpoint definitions #126

Merged

Parkreiner added 2 commits April 30, 2024 14:49

fix: remove configApi from embedded class properties

e370197

fix: update query logic to remove any whitespace

9a359dc

Parkreiner merged commit dd2dc38 into main Apr 30, 2024
4 checks passed

Parkreiner deleted the mes/api-v2-01 branch April 30, 2024 16:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: reorganize API logic and create class/hook for simplifying proxy logic #124

refactor: reorganize API logic and create class/hook for simplifying proxy logic #124

Parkreiner commented Apr 26, 2024 •

edited

Loading

Parkreiner Apr 26, 2024 •

edited

Loading

Parkreiner Apr 29, 2024

Parkreiner Apr 26, 2024 •

edited

Loading

Parkreiner Apr 29, 2024

Parkreiner Apr 26, 2024

Parkreiner Apr 29, 2024 •

edited

Loading

code-asher Apr 29, 2024

Parkreiner Apr 29, 2024 •

edited

Loading

Parkreiner Apr 29, 2024

code-asher Apr 29, 2024

code-asher Apr 29, 2024

Parkreiner Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024

code-asher Apr 29, 2024

Parkreiner Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024

code-asher Apr 29, 2024

Parkreiner Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024 •

edited

Loading

Parkreiner May 1, 2024 •

edited

Loading

Parkreiner May 1, 2024 •

edited

Loading

code-asher May 1, 2024 •

edited

Loading

code-asher Apr 29, 2024

code-asher Apr 29, 2024

Parkreiner Apr 30, 2024

code-asher Apr 29, 2024 •

edited

Loading

Parkreiner Apr 30, 2024

code-asher left a comment

		// Defined here and not in CoderAuthProvider.ts to avoid circular dependency
		// issues

	const enabled = inputs.auth.isAuthenticated && inputs.coderQuery !== '';
	const enabled = inputs.auth.isAuthenticated && inputs.coderQuery.trim() !== '';

refactor: reorganize API logic and create class/hook for simplifying proxy logic #124

refactor: reorganize API logic and create class/hook for simplifying proxy logic #124

Conversation

Parkreiner commented Apr 26, 2024 • edited Loading

Changes made

Notes

Parkreiner Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Parkreiner Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

code-asher Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

code-asher Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Parkreiner May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Parkreiner May 1, 2024 • edited Loading

Choose a reason for hiding this comment

code-asher May 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-asher Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-asher left a comment

Choose a reason for hiding this comment

Parkreiner commented Apr 26, 2024 •

edited

Loading

Parkreiner Apr 26, 2024 •

edited

Loading

Parkreiner Apr 26, 2024 •

edited

Loading

Parkreiner Apr 29, 2024 •

edited

Loading

Parkreiner Apr 29, 2024 •

edited

Loading

Parkreiner Apr 30, 2024 •

edited

Loading

Parkreiner Apr 30, 2024 •

edited

Loading

Parkreiner Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024 •

edited

Loading

code-asher Apr 30, 2024 •

edited

Loading

Parkreiner May 1, 2024 •

edited

Loading

Parkreiner May 1, 2024 •

edited

Loading

code-asher May 1, 2024 •

edited

Loading

code-asher Apr 29, 2024 •

edited

Loading