coder / backstage-plugins

Official Coder plugins for the Backstage platform
40 stars 3 forks source link

Coder plugin: refactor codebase to use API factories #107

Closed Parkreiner closed 6 months ago

Parkreiner commented 7 months ago

Part of umbrella issue #16 Bleeds into #114 a little bit.

These changes are going to take a while, but they are close to done. Adding API factories should require some slightly complicated upfront work, but should make it easier to add new features over time, while making our plugin align more with modern Backstage community practices.

Requirements

Should have

Parkreiner commented 7 months ago

Brain dump from previous message. You don't need to read this unless I suddenly get hit with a bus, in which case, hopefully this is detailed enough


Context

The more I've work with Backstage, the more I've realized that it was a mistake not to use their API factories. When I was first putting the plugin together, I was focused on doing things the "traditional React way", and not the "Backstage way". All the API logic is also thrown into one file, and I think it's getting to the size where it should be split up, now that we've seen a few different use cases play out.

As of this writing, the plugin does not use any custom API factories, which is the main system that Backstage leverages. API Factories are somewhat similar to React Context, in that they're also a dependency injection mechanism, but Backstage is built primarily with factories and not Context. And because the Coder plugin is built mainly with Context, that limits their ability to talk to each other, and let the user swap pieces out when wiring up the plugin.

Parts of an API factory

When wired up correctly, a single API factory consists of three parts. The first two have some pitfalls when trying to show UI state, which I'll highlight in the 'challenges' section below.

  1. The API class itself, which houses all the main logic. The vast majority of these classes are designed to be "function buckets", and aren't designed to be sources of truth on UI state – at least, not by themselves.
  2. The apiRef value, which basically serves as the "key" for the active class instance. Once a class is instantiated, that instance can be accessed at anytime from the React app by calling the useApi hook (e.g., useApi(apiRef)).

    Unfortunately, the useApi name is misleading, as it isn't a React hook at all – it has zero React-specific state. The "hook" grabs the API class instance as a global singleton, and exposes it directly with zero changes.

  3. The site where you plug the API factory in. Backstage actually offers a few different places where you can inject API factories, but for the most part, you'll only need to care about two:
    1. The plugin definition in the plugin.ts file. When you create a plugin with the createPlugin function, you have the option to feed additional API dependencies in as factories. When defined this way, these factories are always part of the plugin, and cannot be changed by users
    2. The packages directory – specifically the apis.ts file that the Backstage scaffolder automatically generates. When factories are injected this way, the user gets to choose which APIs they want. For example, Backstage offers a number of APIs for working with the most popular source control sites, but on our side, we can define a single coderAuthRef value, and let the user decide if they want to associate that with token auth or Coder OAuth.

Benefits to doing a refactor

Refactoring the codebase to (1) split it up, and (2) use these factories should give us the following benefits:

  1. Making it much easier to let us run the Coder plugin in isolation
  2. Giving us obvious, centralized places to put future API functionality (such as the function for calling any arbitrary API call)
  3. Making it easy for the user to to swap in different factories for similar concerns (as mentioned above, if we have a TokenAuth class and an OAuth class, they could use the same coderAuthRef to let the user decide which auth solution they want to use)
  4. Cleaning up code in general, and giving us more ways to do dependency injection during testing
  5. Decreasing the amount of "global-ish" solutions
  6. Extracting an API factory outside React means that we can start to migrate away from React Context, and sidestep Context's potential performance issues

    Up 'til now, I've been very careful about how our CoderProvider's React context has been defined, but there's always a risk that it could still be wired up in a way that causes slow performance. By extracting the main logic outside React, you can make it so that each individual component that cares about the class can subscribe to it individually, without having to go through a shared parent.

  7. We can link API factories together much more directly. For example, we can have a CoderClient API class, which is composed from a CoderAuth API class, and which can receive the auth instance as part of its constructor call. No need to wire these up from within the UI logic.
  8. May remove the need for the duplicate deployment property in the CoderAppConfig type, and could let us migrate all config logic to be YAML-based

Challenges

Challenges in UI code

As mentioned above, the vast majority of API classes are "function buckets". That is, they can be stateful, but:

  1. The majority of the state is defined in the constructor
  2. By and large, a class's only public API properties will be all functions/methods
  3. Backstage doesn't offer any built-in tools for syncing API classes with React state.
  4. Backstage assumes that a React component will only ever need the direct return value of an API function for its UI state. That is, if an API class's method mutates a property in that same class, and other UI components care about about that property, there is no built-in way to have those UI components automatically update. They will be stuck with stale data, because classes are mutable, and React UI is based entirely around immutable values. There is no equivalent of the setState method that React class components have.
  5. Backstage assumes that the vast majority of API calls will be simple on-mount calls that require no unique caching strategy or revalidation/refetching over time. It does not provide any guidance for other use cases.

    Backstage's official recommendation for working with API classes is the useAsync library, which isn't nearly powerful enough for our needs, because it has zero caching/revalidation logic. For example, the current component logic would be outright impossible – we would not be able to automatically detect token changes or workspaces getting added/having their statuses changed.

  6. Even though React is heavily based around passing functions around as values, Backstage has no documentation or protections for making sure that API functions don't lose their this context when they're exposed in the UI (which can happen all too easily if you're not careful).
  7. Newer Backstage APIs are not currently designed for UI needs
    • For example, Backstage has pseudo-deprecated the Config API class in favor of the Discovery API class. Both would let us take the proxy config values in the main YAML file, and make them available in JavaScript. The big problem, though, is that the Config API's methods are synchronous, while the Discovery API's main function is async. We need the values to be available synchronously in UI code, because we're showing parts of the data directly in the UI itself. The Discovery API was designed with the assumption that it would be used solely for async API calls, so we need some kind of workaround to make the values available synchronously

Challenges for testing

Backstage's testing documentation still has a lot of TODO sections, so there isn't any official guidance on using custom API factories, but I know for a fact that you can inject them during testing. It's part of the reason why API factories exist at all. I don't expect this part to take too much time, but I will need to look through the source code to see how the classes from the core Backstage plugins are wiring things up

Proposed solution for initial refactor

I already have a separate branch with a lot of the changes already made, but my solution consists of a few parts:

  1. Split up our main API file into an API folder with smaller, more focused files
  2. Create a coderAuthApiRef value and a "shared auth interface" that has methods that could be shared by both a CoderTokenAuth class, a hypothetical CoderOAuth class, or any other custom auth solutions we build in the future
  3. Create a CoderTokenAuth class, and migrate the vast majority of the current auth token logic into that
    • The class must be defined with a subscription mechanism that lets React components subscribe to state changes in the class. Whenever a token is changed by the user, or the validity of the token changes, those changes should be done via mutation in the class itself. But in response to these mutations, the class should then dispatch "immutable snapshots" of the new state to all React components. This is the design approach that a lot of React state management tools use under the hood (via React's built-in useSyncExternalStore hook)
    • All public methods must be defined in a way that ensures that they cannot lose their this binding when passed around a React UI
  4. Create a useCoderTokenAuth hook that handles the tricky steps of wiring up the CoderTokenAuth class with React Query (particularly its loading/error/success states, and its automatic revalidation)
    • The hook must not have any critical state be initialized within the hook itself. All critical state should be defined outside React so that it can be shared across multiple React component instances.
    • Put another way, if you call useCoderTokenAuth in multiple components, that shouldn't cause anything to blow up, or cause each component that called the hook to fight each other by dispatching different state changes. There should still be one main source of truth (that just happens to be defined outside React)
  5. Create a CoderClient API class and a coderClientApiRef value for injecting the class into Backstage as a factory
    • The Client class should receive an instance of the auth interface as part of its constructor. It shouldn't know which auth solution it has – just that it can rely on the methods defined by the shared interface
    • And because the client will always have an auth instance, none of the UI components will need to pass it in manually anymore (like we're doing right now). UI logic should get at least a little cleaner
    • This class will likely be able to get away with being a "function bucket", so it won't need a subscription mechanism like CoderTokenAuth will, but all public methods should still be defined in a way that ensures they can't lose their this context
  6. Make a useCoderClient custom hook that simplifies wiring up useApi and coderClientApiRef for users and plugin developers; make sure the plugin exports it
  7. Embed the CoderClient class at the plugin definition level to ensure that it's always defined, and so that the end-user doesn't have to worry about hooking it up
  8. Export out the coderAuthRef and CoderTokenAuth values, and add user documentation for getting those set up
    • The idea is that once we add support for Coder OAuth, we can define that as a separate class, and then the user can choose whether they want to hook up CoderTokenAuth or CoderOAuth to the ref
  9. Add migration documentation to ensure that users know what has changed, and how to go from the previous plugin setup to the new one
Parkreiner commented 7 months ago

Update on this: right now, I have a branch that tackles this issue and #114 at once. But with the Coder SDK issues, now both are blocked.

I could split these up, but without the Coder SDK, I don't know if API factories have enough immediate impact to justify undoing some of the work, and then redoing it once the SDK is available. Happy to change course if there's disagreement, but I think this can be parked for a little bit. It won't block other Backstage issues like the Coder buttons