chore(adr): Use REST/auth-client over GQL when convenient in FxA #18138
Conversation
Because: * This conversation has been ongoing for a while and given the mixed state of GQL and auth-client in FXA we want to document pros/cons of our options and what we want to do moving forward This commit: * Adds the ADR for this decision closes FXA-10893
|
|
||
| ## Context and Problem Statement | ||
|
|
||
| In [ADR 16](https://github.com/mozilla/fxa/blob/main/docs/adr/0016-use-graphql-and-apollo-for-settings-redesign.md), 4 1/2 years ago, FxA decided to use GraphQL and Apollo Cache with the Settings redesign project for developer tooling, client-side caching, and performance gains expectations. We began by layering GQL on top of our existing REST service for faster initial development in GQL and to mitigate risk around going "all in" on a novel technology for FxA, using auth-client (our client that talks to auth-server to connect to our REST endpoints) in graphql-api. |
| - Good, because it eliminates the complexity of maintaining two data-fetching paradigms, simplifying the codebase to a single approach | ||
| - Good, because we want to do some back-end refactoring to use `libs/` regardless | ||
| - Bad, because moving away from GQL later if desired may be challenging | ||
| - Bad, because the level of effort to complete the back-end refactor is high and also requires front-end refactoring away from auth-client, both of which may not be useful to us if we move to NextJS in the future |
There was a problem hiding this comment.
I'd also note that using GQL in this way (vs skipping auth-client and auth-server and directly connecting to libs) adds unnecessary development complexity (significantly increasing dev time), introduces additional points of failure, increases latency and complicates error-handling.
|
|
||
| A. Use GQL for all new network requests, continue creating GQL resolvers that use auth-client, and plan to deprecate auth-client through client-side and server-side refactoring | ||
| B. Use auth-client for all new network requests, or prefer auth-client and only use GQL where already set up | ||
| C. Use auth-client for all new network requests, and plan to strip out GQL and use auth-client everywhere |
There was a problem hiding this comment.
Should we also note the pros and cons of going "all-in" on GraphQL?
There was a problem hiding this comment.
I'll take a look at what I already have noted in that "deprecate auth-client" google doc, maybe I can at least note and refer to it if someone's interested in learning more. A is like, step 1 of going all-in, and not sure I want to expand this ADR out much more.
|
|
||
| The varied usage of GQL and auth-client in our front-end feels fractured, we are not reaping the benefits of the end-to-end type safety, and our graphql-api still overfetches data it requests from auth-server via auth-client. All of this can be mitigated by going "all-in" on GraphQL with back-end refactoring - e.g. migrating pieces of auth-server into our new `libs/` directory and using them directly in graphql-api instead, and essentially deprecating auth-client. See our [Google doc "fxa-auth-client usage in FxA"](https://docs.google.com/document/d/1cXoINF50KIUvR1tp22qdZCS3YwN6kMNtdD2kvWN6xPk/edit) created in Q4 2023 for more details on this and [the epic made for it](https://mozilla-hub.atlassian.net/browse/FXA-8633). | ||
|
|
||
| Additionally, we don't have documentation on when to use our existing state management tools - Apollo Cache, location state, local storage, and "sensitive data client". On top of this, FxA desires a move to NextJS (or similar framework) at some point in the future which will require looking at our network requests and state management holistically. We don't want to spend a lot of time refactoring only to refactor to another approach later. |
There was a problem hiding this comment.
Considering the intent to move to server-side rendering, should we clearly note in this ADR how we could ease that transition with decisions made today? Perhaps clarify why moving away from GraphQL would be more complex?
|
Thanks for writing this! |
|
|
||
| ### Negative Consequences | ||
|
|
||
| - We'll still have to manually update Apollo Cache |
There was a problem hiding this comment.
Do any of other options eliminate this? Would be nice to call that out if so.
| - Considerations for the future - we currently would like to move to NextJS eventually | ||
|
|
||
| ## Considered Options | ||
|
|
There was a problem hiding this comment.
Not sure if it's just Github, but this list is not being rendered as a list.
| - Clarity of direction while maximizing flexibility for developers for now | ||
| - Reduced complexity and cognitive load | ||
| - Increased developer velocity due to not needing to set up GQL resolvers | ||
| - By using auth-client we'll be directly querying auth-server instead of essentially using our GraphQL server as a proxy to talk to auth-server for us (which seems preferable especially for simple mutations) |
| ### Negative Consequences | ||
|
|
||
| - We'll still have to manually update Apollo Cache | ||
| - Potential loss of GQL benefits, including previous GQL refactoring investments |
There was a problem hiding this comment.
Heh, I feel like some of this is sunk-cost fallacy.
|
|
||
| This worked fairly well inside of account settings where we could reduce network requests and it was clear what data we were querying, accessing, and updating client-side. However, in [#8138](https://github.com/mozilla/fxa/pull/8138), the FxA team proposed and later implemented a refactoring effort to move API logic out of components in favor of an [`Account` class abstraction](https://github.com/mozilla/fxa/blob/2d130ed6a0cffa2394a6e1f13a6992140630dd84/packages/fxa-settings/src/models/Account.ts). It may not have been clear at the time because we were also still using auth-client in some places likely due to convenience, but this pattern requires us to manually update Apollo Cache with every request since they're called outside of React components. While we have mitigated some of this by moving to container components ([see ADR 39](https://github.com/mozilla/fxa/blob/main/docs/adr/0039-use-react-container-component-abstraction.md)), we still have to manually update Apollo Cache with every auth-client call. | ||
|
|
||
| Since expanding our React work beyond account settings, FxA's experience with GQL has been a mixed bag. While nice sometimes in the front-end, it has increased complexity of some projects like key stretching since we're using both GQL and auth-client, increased some refactoring and new feature scopes due to needing to set up GQL resolvers, and our manual updating of Apollo Cache has caused more than one bug. |
There was a problem hiding this comment.
Maybe also mention that it's simply more work. Adding something to the front end that requires new server data essentially means: Updating a gql resolver, creating a new type in gql, creating a new corresponding type in settings, creating very specific, opinionated apolo mocks, create unit tests for the resolver. Also, if we are just trying to access functionality in the auth server, we might also have update the auth client, and/or modify auth server endpoints. All this extra effort slows down development and depending on how the resolver fetches data may not even lead to any performance gain.
|
@chenba @vpomerleau @dschom Thank you all for the feedback! I pushed an update based on comments and left it as a separate commit for easier viewing. I'm happy to make any further edits.
😛 I ended up putting |
Because:
This commit:
closes FXA-10893