Skip to content

docs: add API documentation for Coder plugin #15

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 36 commits into from
Mar 11, 2024
Merged

Conversation

Parkreiner
Copy link
Member

@Parkreiner Parkreiner commented Mar 5, 2024

Work in progress still.

Changes made

  • Adds API docs
  • Update main README to link to API docs
  • Moves API docs up one level to be at the root of the ./docs directory
  • Additional fixes
    • Makes sure all values referenced in the API documentation are properly exported (had to remove forwardRef from some of these, because Backstage's export system isn't flexible enough to know how to handle them)
    • Made a small logic fix to how workspace parameter data from CoderProvider and each individual catalog-info.yaml file are combined (only caught this while writing out notes for how the functionality was supposed to work)

Things left to do

  • Write docs for the individual CoderWorkspacesCard sub-components
  • Make sure that all sub-components are properly exported.
  • Double-check the source code for useApi to verify if there are any situations when it would realistically throw, and then update the docs for CoderErrorBoundary

Notes

@Parkreiner Parkreiner self-assigned this Mar 5, 2024
Copy link
Member

@bpmct bpmct left a comment

Choose a reason for hiding this comment

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

Thoughts in putting these all in ./docs instead of ./docs/api-reference?

@Parkreiner
Copy link
Member Author

@bpmct Yeah, perfectly fine with that. No strong feelings about file structure

Copy link
Member

@bpmct bpmct left a comment

Choose a reason for hiding this comment

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

Another question, which is related to "Example of using full Coder API" in #14

Is there a clean way for a plugin developer to use the Coder API/token collector logic to make custom requests to the Coder API? To list templates using the users' token, for example.

My optimal UX (psuedocode:

const CustomComponent = () => {

const Coder = useCoderAPI()

if (Coder.userLoggedOut) {
return <TokenLoginComponent />
} else {
   return (<ul>
   {{Coder.APIRequest("/templates").map((template) {
     return <li>{template.name}</li>
    })}}
  </ul>
}
  
}

of course the naming I picked isn't fantastic, but essentially letting plugins devs write their own integrations

@Parkreiner
Copy link
Member Author

Parkreiner commented Mar 6, 2024

@bpmct Not yet. To be honest, there really isn't a perfect way to do it – there's at least some kind of trade-off. I feel like there's two main approaches:

  1. Import the Coder types through NPM (Asher was mentioning that this was an option the other day)
    • This would give us access to the full Coder API pretty quickly, but there wouldn't be much type-safety. We'd have to do a lot of unsafe type-casts, because the types wouldn't exist at runtime
    • But this wouldn't be too different from the approach we use for the main Coder app codebase – as long as we continue to generate types correctly, there shouldn't be any issues. If there are issues with the type casts, though, hopefully we'd be able to detect those quickly in Dogfood
    • Plus, I feel like if we have types published, then there is an expectation that the types are polished. So if there are any issues around that, that'd probably be our cue to get those tightened up
    • My big concern, though, is the testing, and whether it makes sense to add at least one test case for every single endpoint that we expose
  2. Continue using the current approach, which involves distrusting every response we get, and parsing/validating it before we bring it into the main UI
    • This is the only way to get 100% type-safety, but the maintenance cost is going to explode exponentially as we keep adding more endpoints
    • To be clear, the logic basically works by taking the API response, parsing the structure of the value, and removing any properties that we haven't pre-approved
    • But the trade-off is that right now, we're only allowing a subset of the workspace properties. Meaning that even in the current workspace code, we're getting a response with the workspaces, but then we're filtering a lot out.
      • Just because there are so many related and interconnected types, I don't think this approach has legs

But I do think that (1) is the only approach that makes sense from a flexibility/maintenance standpoint, and it's basically the next new feature I want to add to the plugin.

I would like the developer experience to be nice, and for there to be good auto-complete support, though. So two immediately approaches come to mind:

  1. Have an extra layer of indirection, and avoid exposing the raw endpoint
    const workspacesQuery = useCoderApi({
       type: "GET",
       resourceType: "workspace" // or "template"/"users"/any other resource type
    });
    I'm not sure if this is a concern for us, but it would remove the reliance on specific API paths. We could change them on our end and in the plugin without the user needing to change their code
  2. Expose the API endpoints directly, but still associate them with specific types:
    // Return type is dynamically inferred based on the arguments passed to workspaces
    const workspacesQuery = useCoderApi({
       type: "GET",
       url: "/workspaces", // Also has type-checking here to catch typos in endpoints
    });
    The second is doable through TypeScript Magic, but it might end up being an absolute nightmare to maintain. I've gotten pushback on code like this for getting too in the weeds of TypeScript – even without starting anything, I know for a fact the code for Backstage would be at least 10x more complicated

The above code assumes that we're using React Query, which I feel has been really important for making all the authentication and other requests maintainable. But I feel like there's no one-size-fits-all caching strategy we can use for every single endpoint. We could expose the query key directly to let users manage their own caching, but I'll have to see if there's a better option.

With how the proxy is set up right now (and I'm hoping we can keep this if we remove the proxy config from the .yaml file), any request to the Coder API should go through no problem, so maybe we could just expose a "buyer beware" hook for getting a function that makes any arbitrary API request. Put the onus of caching, limiting re-renders, stale closures, and all the other usual issues squarely on them.

@bpmct
Copy link
Member

bpmct commented Mar 6, 2024

From the discussion: let's avoid type safety and expose the API directly since we have autogenerated API docs. Also, can we pass all properties (e.g. request method, request body, anything) into something like fetch or Backstage's equivalent? I'd hate for us to be a subset of fetch, for example and miss something critical for some requests such as the body or adding additional headers

@bpmct
Copy link
Member

bpmct commented Mar 6, 2024

Can you also edit the README to uncomment and add proper links to the docs as part of this PR?

@Parkreiner
Copy link
Member Author

Parkreiner commented Mar 6, 2024

Can do to both of them. I should be able to make it so that the function you get back uses fetch under the hood and has the exact same type signature

(This is sort of a curse – it'd be nice if we could make it so that you can't accidentally pass in the coder header, but I can make it so that there's a quick input check to see if that property was defined, and only let requests through that don't have it – otherwise the user gets an error)

@Parkreiner Parkreiner marked this pull request as ready for review March 8, 2024 15:15
Comment on lines +55 to +58
const activeConfig = {
...appConfig.workspaces,
...(entityConfig ?? {}),
};
Copy link
Member Author

Choose a reason for hiding this comment

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

With how this was before, you would basically only have one config or the other, even though entityConfig wouldn't be guaranteed to be fully filled-out

Now it's using the appConfig value as the base, and only overwriting it with the properties that definitely exist

Copy link
Member

@bpmct bpmct left a comment

Choose a reason for hiding this comment

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

LGTM! Down the road, it might be nice to have an examples/ folder as well in here! For example:

  • examples/CustomWorkspacesCard.tsx - example of somebody importing components instead of using CoderWorkspacesCard
  • examples/Skaffolder.tsx- example using Coder inside the Skaffolder
  • examples/CustomAPI.tsx example of a component making custom API requests (e.g. listing templates)

@bpmct
Copy link
Member

bpmct commented Mar 11, 2024

These docs are extremely nice. And on second thought,

Down the road, it might be nice to have an examples/ folder as well in here!

I imagine this would probably be best in the plugin root instead.

@Parkreiner Parkreiner merged commit cc674de into main Mar 11, 2024
@Parkreiner Parkreiner deleted the mes/coder-api-docs branch March 11, 2024 15:37
Parkreiner added a commit that referenced this pull request Mar 11, 2024
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