Nick-Lucas commented 3 months ago

145 asks for a way to transform the underlying data of a response when Date is set on the type itself.

For some use cases this would be inapropriate since it could bloat bundle size, but for many this is desirable behaviour. Bundle size could also be optimised a bit, but generating raw code to convert types is about the most efficient you can get for runtime performance

This PR is a first crack at adding optional transforms to the project.

Proposal:

Extend ApiRequestOptions with readonly responseTransformer?: (data: unknown) => unknown; - this way we can apply a transform to the result inside the CancellablePromise wrapper, so long as we implement a few lines within each client
Extend types: { dates: true } with boolean | "types" | "types+transform" and leave true as just types
Scan all models for date/date-time openapi types, and if matched generate a typescript funtion like export const ModelName = (data: unknown) => ModelName - typescript is really great in this area, a type and a variable can have the same name so long as they're exported from the same file, and typescript will use the correct one contextually
When response Models are aliased, we also alias this transformer with the same name
Whenever a transformer is created/aliased for a model, we also update client.types with hasTransformer=true so client codegen can detect the availability of a responseTransformer

Proposed limitations for the first iteration:

We don't handle multiple success types/codes, transformers will be skipped for those endpoints, and they're believed to be rare situations
We don't handle transforming error responses at all, could come when the above is resolved if necessary
We don't transform inline types in responses, only $ref schemas. This can be improved on later but needs some dedicated discussion on the approach
We don't support 3rd party date-time libs, if well justified could be supported later

Work to do:

[ ] Agree changes to the implementation of this piece (I'm also happy to hand it over if preferred since I've done all the harder stuff with ts factory work)
[x] Flesh out v3-transform.json with more cases: date arrays, arrays with objects containing dates, nested objects in object keys
[x] Write a decent snapshot test suite around the above
[x] Right now type unions are ignored because discriminating between them to apply a transformer feels non-trivial, decide a direction here
[x] Safer handling of situations where multiple openapi results are in the list, this is related to type unions piece, but currently we just take the first one
[x] All generated output formats should support this transforms codegen as only the basic request one is done here - it's mostly just gruntwork
[x] All package/client output formats should support this transforms codegen as only the basic request one is done here - it's mostly just gruntwork
[x] Docs?

The above proposal produces output like the below:

// This file is auto-generated by @hey-api/openapi-ts

import type { CancelablePromise } from './core/CancelablePromise';
import { OpenAPI } from './core/OpenAPI';
import { request as __request } from './core/request';
import { ComplexParamsResponse } from './types.gen';

/**
 * @returns ModelWithPattern Success
 * @throws ApiError
 */
export const complexParams = (): CancelablePromise<ComplexParamsResponse> => __request(OpenAPI, {
    method: 'PUT',
    responseTransformer: ComplexParamsResponse,
    url: '/api/v{api-version}/model-with-date/{id}',
  });

//
// types.gen.ts

// This file is auto-generated by @hey-api/openapi-ts

/**
 * This is a model that contains a some patterns
 */
export type ModelWithPattern = {
  key: string;
  name: string;
  readonly enabled?: boolean;
  readonly modified?: Date;
  id?: string;
  text?: string;
  patternWithSingleQuotes?: string;
  patternWithNewline?: string;
  patternWithBacktick?: string;
};

export function ModelWithPattern(data: any): ModelWithPattern {
  if (data?.modified) {
    data.modified = new Date(data.modified);
  }
  return data;
}

export type ComplexParamsResponse = ModelWithPattern;

export const ComplexParamsResponse = ModelWithPattern;

export type $OpenApiTs = {
  '/api/v{api-version}/model-with-date/{id}': {
    put: {
      res: {
        /**
         * Success
         */
        200: ModelWithPattern;
      };
    };
  };
};

stackblitz[bot] commented 3 months ago

Run & review this pull request in StackBlitz Codeflow.

changeset-bot[bot] commented 3 months ago

⚠️ No Changeset found

Latest commit: 7e4849562668b9a7658b5b1bb9c146500d834da5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
hey-api-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jun 16, 2024 3:24pm

Nick-Lucas commented 3 months ago

Marking this ready for review since I'd like to get feedback on the general direction before completing the TODOs I've written up

mrlubos commented 3 months ago

Hey @Nick-Luca I will have a look but currently busy with other projects for a few weeks

Nick-Lucas commented 3 months ago

Hey @Nick-Luca I will have a look but currently busy with other projects for a few weeks

Thanks! I think so long as I have an idea what your views are on the direction, I can finish it off and dogfood a fork for a while. We have a pretty complex API in my org so plenty of corner cases to discover I'm sure

Nick-Lucas commented 3 months ago

Okay I've left some comments with questions and TODOs, but it's time for dinner here so I'll be back on it tomorrow for the few bits I already know the answer to.

A few details I think we can get away with ignoring initially as they're limitations rather than bugs and I'm wary of this PR getting too big and lasting too long

mrlubos commented 3 months ago

Enjoy your dinner @Nick-Lucas. Do you consider this pull request ready for now?

Nick-Lucas commented 3 months ago

Not quite ready to merge, I spotted at least one certain bug so I'll fix that

Nick-Lucas commented 3 months ago

But definitely ready for some review on the meat of it

Nick-Lucas commented 3 months ago

Okay @mrlubos this is ready I believe. I've updated the RFC in the PR body and also documented the proposed limitations for the first version of this. I've linked it locally to my team's codebase. We have a 13,500 line OpenAPI spec from SpringDoc and it helped me fix one mistake but the rest looks good.

I haven't got my own codebase compiling yet but that's because our BFF+UI are now on fire because a bunch of strings are now dates :D

mrlubos commented 3 months ago

This is amazing @Nick-Lucas, I will review it + write docs

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 11.60542% with 457 lines in your changes missing coverage. Please review.

Project coverage is 71.65%. Comparing base (8cdd1e2) to head (340cbeb). Report is 2 commits behind head on main.

:exclamation: Current head 340cbeb differs from pull request most recent head 7e48495

Please upload reports for the commit 7e48495 to get more accurate results.

Files	Patch %	Lines
packages/openapi-ts/src/compiler/transform.ts	7.30%	241 Missing :warning:
packages/openapi-ts/src/utils/write/transforms.ts	12.41%	127 Missing :warning:
packages/openapi-ts/src/utils/write/services.ts	1.72%	57 Missing :warning:
packages/openapi-ts/src/utils/write/types.ts	13.33%	26 Missing :warning:
packages/openapi-ts/src/utils/write/type.ts	0.00%	4 Missing :warning:
packages/openapi-ts/src/compiler/return.ts	0.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #670 +/- ## ========================================== - Coverage 75.67% 71.65% -4.03% ========================================== Files 74 76 +2 Lines 7400 7895 +495 Branches 692 696 +4 ========================================== + Hits 5600 5657 +57 - Misses 1797 2235 +438 Partials 3 3 ``` | [Flag](https://app.codecov.io/gh/hey-api/openapi-ts/pull/670/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hey-api) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/hey-api/openapi-ts/pull/670/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hey-api) | `71.65% <11.60%> (-4.03%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=hey-api#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

mrlubos commented 3 months ago

@Nick-Lucas since this changes types from string to Date, aren't you going to have an issue where requests now expect Dates according to TypeScript, but since there's no request transformer, we want to send strings?

Nick-Lucas commented 3 months ago

@Nick-Lucas since this changes types from string to Date, aren't you going to have an issue where requests now expect Dates according to TypeScript, but since there's no request transformer, we want to send strings?

Yes because that's how the types emission currently works. Every JS request lib I know will serialise the dates into ISO strings, so I'm not expecting an issue really, unless someone has a format:date-time but is using a custom string format they want to manually serialise to.

I was wondering if we need to flip around generation so that request/response types are generated and pull from the schemas (ie. request could be Date | string and response gets a transformer) as opposed to now where we generate types from the schemas and then map those to the request/response types. Might also help with schema-less types which are inlined on the request/response definitions

mrlubos commented 3 months ago

Do you want to merge and release this as is? Seems e2e tests are failing btw

Nick-Lucas commented 3 months ago

Do you want to merge and release this as is? Seems e2e tests are failing btw

Hm if tests are failing because of these changes I'll need to have a look on Monday, everything I ran was passing but I maybe didn't get to run everything locally.

Looks like the errors here are angular and due to window.api being undefined TypeError: Cannot destructure property 'ComplexService' of 'window.api' as it is undefined. I didn't change anything that I would have thought affects this though so either something weird or the main branch is also failing 🤔

hey-api / openapi-ts

Convert date strings to dates at runtime #670

145 asks for a way to transform the underlying data of a response when Date is set on the type itself.

⚠️ No Changeset found

Codecov Report