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

chore(Coder plugin): update all Backstage code to use preview SDK #131

Merged
merged 24 commits into from
Jun 3, 2024

Conversation

Parkreiner
Copy link
Collaborator

@Parkreiner Parkreiner commented May 24, 2024

Closes #114

Changes made

  • Updated CoderClient
    • Removed mock SDK
    • Removed cleanup method, since it isn't necessary for testing anymore – the SDK itself is easy to instantiate per test case and doesn't require explicit cleanup
  • Updated all types to come from the SDK
    • Removed a lot of Valibot logic that was barely getting used, and was pretty much exclusively for creating custom Coder types
  • Updated all the mock types to be based on mocks that come from Coder core
  • Updated a couple of misc. lines of code to reduce risks of cross-version React type errors
  • Updated useCoderSdk to forward a few more properties that would be helpful to end-users

@Parkreiner Parkreiner self-assigned this May 24, 2024
@Parkreiner Parkreiner changed the base branch from main to mes/vendored-sdk May 24, 2024 22:27
@Parkreiner Parkreiner changed the title chore(Coder plugin): update all Backstage code to use vendored SDK chore(Coder plugin): update all Backstage code to use preview SDK May 28, 2024
@Parkreiner Parkreiner marked this pull request as ready for review May 28, 2024 12:58
@@ -100,50 +96,6 @@ describe(`${CoderClient.name}`, () => {
});
});

describe('cleanupClient functionality', () => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Deleted because the cleanup method no longer exists

Copy link
Member

Choose a reason for hiding this comment

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

Nice!!

Comment on lines +135 to +138
const assetsEndpoint = await urlSync.getAssetsEndpoint();

const allWorkspacesAreRemapped = !workspaces.some(ws =>
ws.template_icon.startsWith(apiEndpoint),
const allWorkspacesAreRemapped = workspaces.every(ws =>
ws.template_icon.startsWith(assetsEndpoint),
Copy link
Collaborator Author

@Parkreiner Parkreiner May 28, 2024

Choose a reason for hiding this comment

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

Typo – didn't realize when I wrote the tests that I was using the wrong endpoint type

Comment on lines +74 to +76
type RequestInterceptor = (
config: RequestConfig,
) => RequestConfig | Promise<RequestConfig>;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Extracted this out to make addRequestInterceptor have slightly less wonky Prettier formatting

Comment on lines +168 to +178
const registerNewToken = useCallback((newToken: string) => {
if (newToken !== '') {
setAuthToken(newToken);
}
}, []);

const ejectToken = useCallback(() => {
setAuthToken('');
window.localStorage.removeItem(TOKEN_STORAGE_KEY);
queryClient.removeQueries({ queryKey: [CODER_QUERY_KEY_PREFIX] });
}, [queryClient]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not a big deal for our use cases, but just because these functions are going to exported to end-users, I though it'd be good to stabilize their memory references, so that people run into fewer useEffect pain points

Base automatically changed from mes/vendored-sdk to main May 31, 2024 15:36
Copy link
Member

@code-asher code-asher left a comment

Choose a reason for hiding this comment

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

Quite satisfying!

@@ -100,50 +96,6 @@ describe(`${CoderClient.name}`, () => {
});
});

describe('cleanupClient functionality', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice!!

axiosInstance: AxiosInstance,
): BackstageCoderSdk {
const baseSdk = new CoderSdk(axiosInstance);
private getBackstageCoderSdk(): BackstageCoderSdk {
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit especially considering this is private, but should we call this makeBackstageCoderSdk? I think get can give off singleton vibes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, good call. I think I've always had a habit of using get for anything that produces a value, but I've been realizing lately that make and create are usually more explicit

@@ -48,7 +48,7 @@ type UrlPrefixes = Readonly<{

export const defaultUrlPrefixes = {
proxyPrefix: `/api/proxy`,
apiRoutePrefix: '/api/v2',
apiRoutePrefix: '', // Left as empty string because code assumes that CoderSdk will add /api/v2
Copy link
Member

Choose a reason for hiding this comment

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

Can we delete this if it is always blank? Or at least, from my brief searching I did not find any instances of apiRoutePrefix being set to something else. Same with assetsRoutePrefix. I wonder if I just missed something though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, you're not missing anything. I guess I decided to keep the property because I thought that maybe down the line, it might make sense to let the prefix be dynamic

Even if we do need that later, though, it should be easy to add back later. I'll get rid of this

@Parkreiner Parkreiner merged commit c245950 into main Jun 3, 2024
2 checks passed
@Parkreiner Parkreiner deleted the mes/vendored-sdk-integration branch June 3, 2024 14:23
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.

Coder plugin: Make Coder SDK available to Backstage users
2 participants