-
Notifications
You must be signed in to change notification settings - Fork 7
refactor: reorganize API logic and create class/hook for simplifying proxy logic #124
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
Changes from all commits
67cb4b3
e53d127
f26184f
b54324b
7daf597
86b5acc
5d14d5a
afd0203
1e16c82
d786e34
063ecf2
1d1ab3c
58566c8
cd5fef6
abfd949
26ae96d
5aabe86
425d50f
04f0f3e
111df43
1575f8e
dc6e75c
fd8b3cb
5af006c
644e632
a667b48
cca343d
aac5a0c
32fb344
e370197
9a359dc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,90 @@ | ||
import { type UrlSyncSnapshot, UrlSync } from './UrlSync'; | ||
import { type DiscoveryApi } from '@backstage/core-plugin-api'; | ||
import { | ||
getMockConfigApi, | ||
getMockDiscoveryApi, | ||
mockBackstageAssetsEndpoint, | ||
mockBackstageProxyEndpoint, | ||
mockBackstageUrlRoot, | ||
} from '../testHelpers/mockBackstageData'; | ||
|
||
// Tests have to assume that DiscoveryApi and ConfigApi will always be in sync, | ||
// and can be trusted as being equivalent-ish ways of getting at the same source | ||
// of truth. If they're ever not, that's a bug with Backstage itself | ||
describe(`${UrlSync.name}`, () => { | ||
it('Has cached URLs ready to go when instantiated', () => { | ||
const urlSync = new UrlSync({ | ||
apis: { | ||
configApi: getMockConfigApi(), | ||
discoveryApi: getMockDiscoveryApi(), | ||
}, | ||
}); | ||
|
||
const cachedUrls = urlSync.getCachedUrls(); | ||
expect(cachedUrls).toEqual<UrlSyncSnapshot>({ | ||
baseUrl: mockBackstageUrlRoot, | ||
apiRoute: mockBackstageProxyEndpoint, | ||
assetsRoute: mockBackstageAssetsEndpoint, | ||
}); | ||
}); | ||
|
||
it('Will update cached URLs if getApiEndpoint starts returning new values (for any reason)', async () => { | ||
let baseUrl = mockBackstageUrlRoot; | ||
const mockDiscoveryApi: DiscoveryApi = { | ||
getBaseUrl: async () => baseUrl, | ||
}; | ||
|
||
const urlSync = new UrlSync({ | ||
apis: { | ||
configApi: getMockConfigApi(), | ||
discoveryApi: mockDiscoveryApi, | ||
}, | ||
}); | ||
|
||
const initialSnapshot = urlSync.getCachedUrls(); | ||
baseUrl = 'blah'; | ||
|
||
await urlSync.getApiEndpoint(); | ||
const newSnapshot = urlSync.getCachedUrls(); | ||
expect(initialSnapshot).not.toEqual(newSnapshot); | ||
|
||
expect(newSnapshot).toEqual<UrlSyncSnapshot>({ | ||
baseUrl: 'blah', | ||
apiRoute: 'blah/coder/api/v2', | ||
assetsRoute: 'blah/coder', | ||
}); | ||
}); | ||
|
||
it('Lets external systems subscribe and unsubscribe to cached URL changes', async () => { | ||
let baseUrl = mockBackstageUrlRoot; | ||
const mockDiscoveryApi: DiscoveryApi = { | ||
getBaseUrl: async () => baseUrl, | ||
}; | ||
|
||
const urlSync = new UrlSync({ | ||
apis: { | ||
configApi: getMockConfigApi(), | ||
discoveryApi: mockDiscoveryApi, | ||
}, | ||
}); | ||
|
||
const onChange = jest.fn(); | ||
urlSync.subscribe(onChange); | ||
|
||
baseUrl = 'blah'; | ||
await urlSync.getApiEndpoint(); | ||
|
||
expect(onChange).toHaveBeenCalledWith({ | ||
baseUrl: 'blah', | ||
apiRoute: 'blah/coder/api/v2', | ||
assetsRoute: 'blah/coder', | ||
} satisfies UrlSyncSnapshot); | ||
|
||
urlSync.unsubscribe(onChange); | ||
onChange.mockClear(); | ||
baseUrl = mockBackstageUrlRoot; | ||
|
||
await urlSync.getApiEndpoint(); | ||
expect(onChange).not.toHaveBeenCalled(); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
/** | ||
* @file This is basically a fancier version of Backstage's built-in | ||
* DiscoveryApi that is designed to work much better with React. Its hook | ||
* counterpart is useUrlSync. | ||
* | ||
* The class helps with synchronizing URLs between Backstage classes and React | ||
* UI components. It will: | ||
* 1. Make sure URLs are cached so that they can be accessed directly and | ||
* synchronously from the UI | ||
* 2. Make sure that there are mechanisms for binding value changes to React | ||
* state, so that if the URLs change over time, React components can | ||
* re-render correctly | ||
* | ||
* As of April 2024, there are two main built-in ways of getting URLs from | ||
* Backstage config values: | ||
* 1. ConfigApi (offers synchronous methods, but does not have direct access to | ||
* the proxy config - you have to stitch together the full path yourself) | ||
* 2. DiscoveryApi (has access to proxy config, but all methods are async) | ||
* | ||
* Both of these work fine inside event handlers and effects, but are never safe | ||
* to put directly inside render logic. They're not pure functions, so they | ||
* can't be used as derived values, and they don't go through React state, so | ||
* they're completely disconnected from React's render cycles. | ||
*/ | ||
import { | ||
type DiscoveryApi, | ||
type ConfigApi, | ||
createApiRef, | ||
} from '@backstage/core-plugin-api'; | ||
import { | ||
type Subscribable, | ||
type SubscriptionCallback, | ||
CODER_API_REF_ID_PREFIX, | ||
} from '../typesConstants'; | ||
import { StateSnapshotManager } from '../utils/StateSnapshotManager'; | ||
|
||
// This is the value we tell people to use inside app-config.yaml | ||
export const CODER_PROXY_PREFIX = '/coder'; | ||
|
||
const BASE_URL_KEY_FOR_CONFIG_API = 'backend.baseUrl'; | ||
const PROXY_URL_KEY_FOR_DISCOVERY_API = 'proxy'; | ||
|
||
type UrlPrefixes = Readonly<{ | ||
proxyPrefix: string; | ||
apiRoutePrefix: string; | ||
assetsRoutePrefix: string; | ||
}>; | ||
|
||
export const defaultUrlPrefixes = { | ||
proxyPrefix: `/api/proxy`, | ||
apiRoutePrefix: '/api/v2', | ||
assetsRoutePrefix: '', // Deliberately left as empty string | ||
} as const satisfies UrlPrefixes; | ||
|
||
export type UrlSyncSnapshot = Readonly<{ | ||
baseUrl: string; | ||
apiRoute: string; | ||
assetsRoute: string; | ||
}>; | ||
|
||
type Subscriber = SubscriptionCallback<UrlSyncSnapshot>; | ||
|
||
type ConstructorInputs = Readonly<{ | ||
urlPrefixes?: Partial<UrlPrefixes>; | ||
apis: Readonly<{ | ||
discoveryApi: DiscoveryApi; | ||
configApi: ConfigApi; | ||
}>; | ||
}>; | ||
|
||
const proxyRouteReplacer = /\/api\/proxy.*?$/; | ||
|
||
type UrlSyncApi = Subscribable<UrlSyncSnapshot> & | ||
Readonly<{ | ||
getApiEndpoint: () => Promise<string>; | ||
getAssetsEndpoint: () => Promise<string>; | ||
getCachedUrls: () => UrlSyncSnapshot; | ||
}>; | ||
|
||
export class UrlSync implements UrlSyncApi { | ||
private readonly discoveryApi: DiscoveryApi; | ||
private readonly urlCache: StateSnapshotManager<UrlSyncSnapshot>; | ||
private urlPrefixes: UrlPrefixes; | ||
|
||
constructor(setup: ConstructorInputs) { | ||
const { apis, urlPrefixes = {} } = setup; | ||
const { discoveryApi, configApi } = apis; | ||
|
||
this.discoveryApi = discoveryApi; | ||
this.urlPrefixes = { ...defaultUrlPrefixes, ...urlPrefixes }; | ||
|
||
const proxyRoot = this.getProxyRootFromConfigApi(configApi); | ||
this.urlCache = new StateSnapshotManager<UrlSyncSnapshot>({ | ||
initialSnapshot: this.prepareNewSnapshot(proxyRoot), | ||
}); | ||
} | ||
|
||
// 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, so it's not being embedded in the class | ||
private getProxyRootFromConfigApi(configApi: ConfigApi): string { | ||
const baseUrl = configApi.getString(BASE_URL_KEY_FOR_CONFIG_API); | ||
return `${baseUrl}${this.urlPrefixes.proxyPrefix}`; | ||
} | ||
|
||
private prepareNewSnapshot(newProxyUrl: string): UrlSyncSnapshot { | ||
const { assetsRoutePrefix, apiRoutePrefix } = this.urlPrefixes; | ||
|
||
return { | ||
baseUrl: newProxyUrl.replace(proxyRouteReplacer, ''), | ||
assetsRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${assetsRoutePrefix}`, | ||
apiRoute: `${newProxyUrl}${CODER_PROXY_PREFIX}${apiRoutePrefix}`, | ||
}; | ||
} | ||
|
||
/* *************************************************************************** | ||
* All public functions should be defined as arrow functions to ensure they | ||
* can be passed around React without risk of losing their `this` context | ||
****************************************************************************/ | ||
|
||
getApiEndpoint = async (): Promise<string> => { | ||
const proxyRoot = await this.discoveryApi.getBaseUrl( | ||
PROXY_URL_KEY_FOR_DISCOVERY_API, | ||
); | ||
Comment on lines
+122
to
+124
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense to me! |
||
|
||
const newSnapshot = this.prepareNewSnapshot(proxyRoot); | ||
this.urlCache.updateSnapshot(newSnapshot); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If we had something like I see the only place we access the synchronous state is 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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?
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?
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
Ah I was thinking about the case where there is an image, but it is a 404.
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. |
||
return newSnapshot.apiRoute; | ||
}; | ||
|
||
getAssetsEndpoint = async (): Promise<string> => { | ||
const proxyRoot = await this.discoveryApi.getBaseUrl( | ||
PROXY_URL_KEY_FOR_DISCOVERY_API, | ||
); | ||
|
||
const newSnapshot = this.prepareNewSnapshot(proxyRoot); | ||
this.urlCache.updateSnapshot(newSnapshot); | ||
return newSnapshot.assetsRoute; | ||
}; | ||
|
||
getCachedUrls = (): UrlSyncSnapshot => { | ||
return this.urlCache.getSnapshot(); | ||
}; | ||
|
||
unsubscribe = (callback: Subscriber): void => { | ||
this.urlCache.unsubscribe(callback); | ||
}; | ||
|
||
subscribe = (callback: Subscriber): (() => void) => { | ||
this.urlCache.subscribe(callback); | ||
return () => this.unsubscribe(callback); | ||
}; | ||
} | ||
|
||
export const urlSyncApiRef = createApiRef<UrlSync>({ | ||
id: `${CODER_API_REF_ID_PREFIX}.url-sync`, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usesassetsRoute
.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could probably rename things to remove that ambiguity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhhhhhhh I probably had the
...Sync
methods in Node'sfs
on my brain. No really good alternatives come to my mind, so I am not opposed to just leaving it.