Skip to content
This repository has been archived by the owner on Feb 13, 2025. It is now read-only.

Route definition based requests #40

Merged
merged 11 commits into from
Feb 13, 2025
Merged

Route definition based requests #40

merged 11 commits into from
Feb 13, 2025

Conversation

kibertoad
Copy link
Collaborator

@kibertoad kibertoad commented Jan 2, 2025

Changes

This is a proposal for a new API for defining REST API requests.

Key idea behind it: backend owns entire definition for the route, including its path, HTTP method used and response structure expectations, and exposes it as a part of its API schemas. Then frontend consumes that definition instead of forming full request configuration manually on the client side.

This reduces amount of assumptions FE needs to make about the behaviour of BE, reduces amount of code that needs to be written on FE, and makes the code more type-safe (as path parameter setting is handled by logic exposed by BE, in a type-safe way).

Checklist

  • Apply one of following labels; major, minor, patch or skip-release
  • I've updated the documentation, or no changes were necessary
  • I've updated the tests, or no changes were necessary

src/client.spec.ts Outdated Show resolved Hide resolved
src/client.spec.ts Outdated Show resolved Hide resolved
"vitest": "^2.0.1"
},
"keywords": ["frontend", "web", "browser", "http", "client", "zod", "validation", "typesafe"]
"name": "@lokalise/frontend-http-client",
Copy link
Collaborator Author

@kibertoad kibertoad Jan 2, 2025

Choose a reason for hiding this comment

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

had to do reformatting, as library repos do not use package-lock file, so CI picked up the latest version of biome and rules file, and CI broke

@leonaves
Copy link

leonaves commented Jan 2, 2025

I really like the fact this reduces passing as many types and tightly couples the types to the definition object, but I'm interested in the API choice of sendByRouteDefinition, as it's a little wordy. Another idea could be that the buildRouteDefinition function (or a similarly named function) also return a send function like:

const { send: updateUser } = buildRouteDefinition({
        method: 'post',
        isEmptyResponseExpected: false,
        isNonJSONResponseExpected: false,
        responseBodySchema,
        requestPathParamsSchema: pathSchema,
        requestBodySchema: requestBodySchema,
        pathResolver: (pathParams) => `/users/${pathParams.userId}`,
      })

? Or are you explicitly trying to avoid something like this?

@kibertoad
Copy link
Collaborator Author

@leonaves Do you have any ideas for a better name? buildRouteDefinition is a bit of a mouthful, I agree 😅

Not including a send function was a deliberate design choice, I wanted to decouple the contract from the implementation, backend and frontend could potentially rely on the same contract, but choose to consume them via different clients.

(contract building part probably will move to shared-ts-libs eventually for the ease of sharing between BE and FE, this is primarily a PoC to illustrate the approach)

@leonaves
Copy link

leonaves commented Jan 3, 2025

Sorry for the slow reply, I've been mulling over this off and on for a little bit—I'm not 100% sold on the idea of the contract needing to be separate. In my example above, I probably should have been more explicit but I was implying that both would be returned:

const { send: updateUser, routeDefinition } = buildRouteDefinition({
        method: 'post',
        isEmptyResponseExpected: false,
        isNonJSONResponseExpected: false,
        responseBodySchema,
        requestPathParamsSchema: pathSchema,
        requestBodySchema: requestBodySchema,
        pathResolver: (pathParams) => `/users/${pathParams.userId}`,
      })

I think it's useful the contract is exposed, but I'm not sure I see the use-case for wrapping that in a different HTTP client library, as it would need to be something that understands the routeDefinition for this library, at which point why not use sendByRouteDefinition from this library? And if we're including that why not also include the send functions themselves? Especially if we are exposing the contract anyway?

I might be missing a use-case that you have already thought of, so I'm sorry if I'm making you over-explain here. I feel like the most common use-case (especially on the front-end) however is going to be importing the route definitions and then importing the sendByRouteDefinition and wrapping it, which feels like an extra layer of abstraction that the frontend doesn't really need to be concerned with.

@ondrejsevcik
Copy link

@kibertoad I would rather go in a direction of having npm library for our REST API (e.g. autopilot-rest-api). The library could then expose methods and types to retrieve data from our BE services. This could be used eventually by 3rd parties as well. And the underlying implementation (whether it is using fetch, wretch or something else is irrelevant).

For inspiration

@kibertoad
Copy link
Collaborator Author

kibertoad commented Jan 7, 2025

@leonaves @ondrejsevcik Main reason why I'm hesitant to inverse the structure and have BE own the actual client used to send requests is losing the important part of the control on the FE side.

Let's consider few examples of what we can do due to the fact that the actual client is a wretch instance, owned by FE itself:

  1. variable granularity error handling, using wretch catchers. You can create a single global wretch instance with universal catchers for errors we don't care about, then attach more specific catchers to further increments of wretch instance, that could be used for one or multiple calls;

  2. flexible auth handler (see helper methods). You can create a method getClientWithAuthentication, that would be returning a wretch instance with a fresh auth token

  3. powerful addon and middleware mechanisms, that allow to customize client further either for app, or for a subset of calls.

All of these things are no longer directly possible if we just create a unified client SDK. Since SDK cannot and shouldn't know how you are handling your errors and auth, you will have to fallback to manual manipulation of parameters for every request, so you'd need to put in the correct Authorization header directly into your call's headers, and do manual if (reply.statusCode > 499) { throw new Error() }

If we find that to be an acceptable tradeoff, and we are fine with the extra boilerplate, then ts-rest is likely to be our best option, it offers an experience pretty close to what both of you are suggesting.

@szymonchudy We could also use your perspective on this.

@szymonchudy
Copy link

Thanks for providing this proposal. Here's my take:

Centralizing route definitions and exposing them as ready-to-use components seems like a great approach to improving the separation of concerns, reducing errors, enhancing maintainability, and boosting usability.

I’m also fully on board with keeping the wretch API on the frontend rather than adopting an SDK-like approach. Maintaining that flexibility feels important, especially for SSR scenarios where we still “don’t know what we don’t know”. Having control over the client, for example to tailor error handling, makes sense to me.

That said, I'm not entirely convinced we need this at our current scale. Our existing abstractions already do the heavy lifting for us, and while centralizing API definitions would improve things, the added complexity may not be worth it yet.

Thanks for pinging me @kibertoad. I missed that one. If we decide to implement this, I'm also happy to play with the types, as I experimented with generics recently.

@kibertoad
Copy link
Collaborator Author

I’m also fully on board with keeping the wretch API on the frontend rather than adopting an SDK-like approach. Maintaining that flexibility feels important, especially for SSR scenarios where we still “don’t know what we don’t know”. Having control over the client, for example to tailor error handling, makes sense to me.

That's actually a really good point - we also don't know how we are going to manage state. E. g. one way to use ts-rest is by applying react-query directly, which is a good idea if we use react-query, but we may end up going with RR7 state management at some point instead.

while centralizing API definitions would improve things, the added complexity may not be worth it yet.

Can you clarify this comment? If we are talking about exposing route definitions from the BE side, wouldn't it decrease the complexity rather than increase it, because there will be less things to know about and track on FE, and it would be making all the internal calls simply by making a 100% type-safe TS call? (as opposed to manually providing path and resolving path params, as it is now)

@szymonchudy
Copy link

szymonchudy commented Jan 8, 2025

Can you clarify this comment? If we are talking about exposing route definitions from the BE side, wouldn't it decrease the complexity rather than increase it, because there will be less things to know about and track on FE, and it would be making all the internal calls simply by making a 100% type-safe TS call? (as opposed to manually providing path and resolving path params, as it is now)

Our current solution works well and follows a typical pattern – the FE provides the path, method, schemas, etc. This is the workflow everyone is familiar with. Moving this responsibility to the backend is a slight paradigm shift that always needs time to settle. Also, during the migration period, we'd need to maintain both approaches.

While I see the benefits, I'm wondering if the impact justifies the extra work needed for migration and maintenance, considering our current (still small) scale. This isn't a "no" – just thinking out loud. 😊

By the way, this raises a question: Are we planning to eventually replace the current approach with this new one, or will we maintain both?

@kibertoad
Copy link
Collaborator Author

Are we planning to eventually replace the current approach with this new one, or will we maintain both?

If we decide to adopt a new approach (either this PR or the https://github.com/lokalise/polyglot-service/pull/1188), I'd say that the goal will be to eventually migrate everything over to it. It doesn't make sense to maintain two different ways to do the same thing, when we consider one of them to be better.

While I see the benefits, I'm wondering if the impact justifies the extra work needed for migration and maintenance, considering our current (still small) scale. This isn't a "no" – just thinking out loud.

That I don't have a strong opinion on. My only point is that if we want to do the change, now is a good time - before we started implementing all the new frontends.

@laurismikals
Copy link

As a FE developer I only care about the data that I need and I care not about how that data is requested. Essentially I need a function to call, need to know the params and need to know the type of the data that will be returned. I am all for this change.

@szymonchudy
Copy link

I contacted everyone involved, and here’s the summary:

There are three options on the table:

  1. @kibertoad proposition - defining the route definitions on the BE side
    • Builds on top of what we already have
    • Not that big of a difference: basically moving -api.ts files from FE to BE, making BE engineers responsible for defining routes
      • FE doesn't have to be aware of schema
    • Very close to the current implementation and easy migration.
  2. @ondrejsevcik proposition - SDK-like approach as a separate NPM package
    • Ondrej believes our API definitions, especially after implementing the proposal, would be a valid candidate for a separate package.
  3. @CatchMe2 proposition - ts-rest (PR with POC)
    • Mateusz values ts-rest for its clarity, thorough documentation, and strong type safety, particularly around handling different response status codes.
    • He appreciates its server-to-server communication capabilities, OpenAPI plugin integration, and straightforward contract definition.
    • While he did invest significant effort into a proof of concept, his points remain largely objective and highlight ts-rest’s technical strengths.
  4. @laurismikals perspective (based on the comment above) supports the proposal strongly, primarily from a frontend engineering standpoint. He seems to welcome any change that reduces frontend development effort, which I read as a strong yes for any of the solutions above.

Thought process

  • While I don't see this as a major pain point currently, I definitely see value in moving route definitions closer to schemas. This would make the frontend less dependent on ZOD schemas, effectively reducing the need for all the -api.ts files (route definitions can be used directly in queries).
  • After further consideration, I don't believe SSR changes our paradigm enough to significantly impact these decisions. All solutions would work similarly.
  • While ts-rest and RPC concepts are appealing, we've already implemented many of their benefits in a more modular way. Adopting this implementation raises concerns about its complexity and the extensive changes needed – changes that would be difficult to reverse, particularly if we adopt their abstractions like the React Query wrapper. The migration process would also be challenging.
  • However, Mateusz's points and proof of concept are compelling and worth exploring further.
  • Regarding the separate package proposal, I remain unconvinced. Currently, API definitions are owned by specific teams/domains, which feels like an appropriate separation of concerns. The route definitions fit naturally within this structure. Maintaining a separate repo/package seems like too much overhead at this point and would likely decrease developer experience and productivity without offering substantial benefits.

Recommendation

To wrap it up, I recommend implementing route definitions as proposed by Igor. This is a relatively low-risk endeavor that could serve as a stepping stone toward more sophisticated solutions.

In parallel, I suggest implementing Mateusz's proof of concept in Polyglot with a defined timeline (e.g., 3-6 months). After this period, we can evaluate its impact and either revert Polyglot to the route definition solution or move forward with broader ts-rest adoption.

Copy link

@ntrpilot-lokalise ntrpilot-lokalise left a comment

Choose a reason for hiding this comment

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

Have you considered the wretch addon mechanism for the API definition?

And example of this might be:

export const SomePropsSchema = z.string()
export type SomeProps = z.infer<typeof SomePropsSchema>

export interface MyApi {
  doSomething: <T extends MyApi, C, R>(this: T & Wretch<T, C, R>, prop: SomeProps) => this
}

export const myApi: () => WretchAddon<MyApi> = () => {
  return {
    wretch: {
      doSomething(prop) {
           return this.url('/some/path') // Build the request
      },
    },
  }
}

And then a client of the API would look something like this

const client = wretchClient.addon(myApi)

const value = await client.doSomething('The string').get();

@kibertoad kibertoad changed the title proposal: route definition based requests Route definition based requests Feb 2, 2025
@kibertoad kibertoad marked this pull request as ready for review February 12, 2025 16:49
Copy link

@szymonchudy szymonchudy left a comment

Choose a reason for hiding this comment

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

LGTM, can't wait to see it flying 🚀

@kibertoad kibertoad merged commit 83c5423 into main Feb 13, 2025
6 checks passed
@kibertoad kibertoad deleted the feat/route-definitions branch February 13, 2025 07:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants