Convert date strings to dates at runtime

[Nick-Lucas]

#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)
• Flesh out v3-transform.json with more cases: date arrays, arrays with objects containing dates, nested objects in object keys
• Write a decent snapshot test suite around the above
• Right now type unions are ignored because discriminating between them to apply a transformer feels non-trivial, decide a direction here
• 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
• All generated output formats should support this transforms codegen as only the basic request one is done here - it's mostly just gruntwork
• All package/client output formats should support this transforms codegen as only the basic request one is done here - it's mostly just gruntwork
• 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]

Run & review this pull request in StackBlitz Codeflow.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 7e48495

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]

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]

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]

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

[Nick-Lucas]

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

[mrlubos]

Leaving a few clarifying comments to start off

[Nick-Lucas]

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]

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

[mrlubos]

Would be nice to add proper types here

[Nick-Lucas]

There's not much point, it's an internal function called by a part of the code that doesn't know the type because it's a response body, so you wouldn't get any compiler benefits. The mutations also benefit from any because we don't have to do any casting during the transform, though that might not be an issue

I'm treating these similar to validation/parser logic in my mind. For instance Zod.parse is always unknown->MyType albeit I'll cede that it checks every key in that situation

[mrlubos]

I was thinking it could help with catching incorrect transforms in tests... why not use unknown over any btw?

[Nick-Lucas]

You could be right, there's no harm in declaring the type on the input and I can't think of a reason it should cause compile issues unless there's a real issue in the transform. I was mostly just going after the validator/parser pattern that tools like Zod use, but it is slightly different here

There is a good reason to use any over unknown though, which is unknown means you can't chain up properties without do a "x" in y check or using a type assertion first so the codegen would be way more verbose and difficult to generate. Any is like disabling typescript

I'm happy to switch things around to take the expected type as input. The main impact would be on the request files but I suspect since their type definition is unknown -> unknown that will all work fine

[Nick-Lucas]

OH actually, I remember why I did it this way - static typed input would be nice, but I didn't want any risk of an unexpected response causing a property x of undefined type error, and so every single access chain uses the optional chaining operator. If we gave it a static input type then is there a chance prettier/eslint could strip the "unnecessary optional chaining" since the input type is known and properties in each chain aren't technically optional?

[codecov]

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.

❗ 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 ⚠️
packages/openapi-ts/src/utils/write/transforms.ts	12.41%	127 Missing ⚠️
packages/openapi-ts/src/utils/write/services.ts	1.72%	57 Missing ⚠️
packages/openapi-ts/src/utils/write/types.ts	13.33%	26 Missing ⚠️
packages/openapi-ts/src/utils/write/type.ts	0.00%	4 Missing ⚠️
packages/openapi-ts/src/compiler/return.ts	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            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	Coverage Δ
unittests	71.65% <11.60%> (-4.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mrlubos]

@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]

@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]

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

[Nick-Lucas]

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 🤔