chore(adr): Use REST/auth-client over GQL when convenient in FxA #18138
+95
−0
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| # Use REST (auth-client) over GQL when | ||
|
|
||
| - Status: proposed | ||
| - Deciders: Lauren Zugai, Ben Bangert, Dan Schomburg, [you?] | ||
| - Date: 2024-12-11 | ||
|
|
||
| ## 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. | ||
|
|
||
| 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. |
||
|
|
||
| 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? |
||
|
|
||
| This brings us to our question: what do we want to do in the meantime about the state of GQL and auth-client in FxA's front-end? | ||
|
|
||
| ## Decision Drivers | ||
|
|
||
| - Level-of-effort to get us consistent using one approach | ||
| - Ease of use, simplicity, and developer velocity | ||
| - 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. |
||
| 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. |
||
|
|
||
| ## Decision Outcome | ||
|
|
||
| Chosen option: B, "Use auth-client for all new network requests, or prefer auth-client and only use GQL where already set up" because by standardizing on auth-client for now or at least using it when it is convenient, we minimize overhead in setting up and maintaining GQL mutations/resolvers while leaving open the possibility of moving to a framework like NextJS later, where we can reevaluate our data-fetching and state strategies more holistically. | ||
|
|
||
| This options allow engineers the most flexibility to get tasks out the quickest way, especially if we can create a DRY wrapper for auth-client like what's described in [FXA-10861](https://mozilla-hub.atlassian.net/browse/FXA-10861). | ||
|
|
||
| Additionally, while we started a [state management/architecture doc](https://docs.google.com/document/d/17dCwjxhxBKXDclkydmPHD91Pj3suIFihR6fZw6m126Q/edit?tab=t.0#heading=h.3neqekw0gen1) in 2023, we should revisit and better finish this documentation to clarify when to use which tool for state management. | ||
|
|
||
| ### Positive Consequences | ||
|
|
||
| - 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 | ||
|
There was a problem hiding this comment. Do any of other options eliminate this? Would be nice to call that out if so. |
||
| - 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. |
||
| - We'll continue to have a mixed usage of GQL and auth-client in our client-side with no immediate plans to mitigate the inconsistency | ||
| - If we change our minds later, we'll have more factoring work to do to deprecate auth-client. However, it's worth noting FxA already has a very mixed usage of auth-client and GQL | ||
|
|
||
| ## Pros and Cons of the Options | ||
|
|
||
| ### Option 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 | ||
|
|
||
| - Good, because we'll finally be able to realize the full benefits of using GQL | ||
| - 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. |
||
|
|
||
| ### Option B: Use auth-client for all new network requests, or prefer auth-client and only use GQL where already set up | ||
|
|
||
| - Good, because it reduces some complexity right now and allows for the most flexibility for engineers when working through tasks | ||
| - Good, because it makes mocking in front-end tests easier | ||
| - Good, because we will avoid laborious back-end and front-end refactoring tasks now in favor of other work and revisiting a plan for consistency when we move to NextJS | ||
| - Bad, because we'll continue to have a mixed usage of GQL and auth-client in our client-side with no immediate plans to mitigate the inconsistency | ||
| - Bad, because it does not further leverage GQL’s potential benefits or capitalize on our previous GQL investments | ||
|
|
||
| ### Option C: Use auth-client for all new network requests, and plan to strip out GQL and use auth-client everywhere | ||
|
|
||
| - Good, because it eliminates the complexity of maintaining two data-fetching paradigms, simplifying the codebase to a single approach | ||
| - Good, because it makes mocking in front-end tests easier | ||
| - Bad, because the level of effort to strip out GQL from our client-side code and remove Apollo Cache would be high and may not be the best use of engineering time until a future framework decision (like moving to NextJS) is made | ||
| - Bad, because it does not further leverage GQL’s potential benefits or capitalize on our previous GQL investments | ||
|
|
||
| ## Links | ||
|
|
||
| - Previous ADRs: | ||
| - [Use GraphQL in settings redesign](https://github.com/mozilla/fxa/blob/main/docs/adr/0016-use-graphql-and-apollo-for-settings-redesign.md) | ||
| - [Use container component pattern](https://github.com/mozilla/fxa/blob/main/docs/adr/0039-use-react-container-component-abstraction.md) | ||
| - [Never finished doc](https://docs.google.com/document/d/1cXoINF50KIUvR1tp22qdZCS3YwN6kMNtdD2kvWN6xPk/edit) for fxa-settings front-end architecture | ||
| - [FXA-10861](https://mozilla-hub.atlassian.net/browse/FXA-10861) - Create auth-client wrapper | ||
| - [FXA-8633](https://mozilla-hub.atlassian.net/browse/FXA-8633) - Epic for moving away from auth-client in graphql-api | ||
| - [FxA + NextJS doc](https://docs.google.com/document/d/1eTWxFclGf_l0NwYefsP0zey8Q9CfrJcmMEZ5XdSXqOY/) | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
4 1/2 years ago 👀