Giter Site home page Giter Site logo

Comments (31)

seriouslag avatar seriouslag commented on May 24, 2024 5

@AnderssonPeter that's why it's disabled by default. Please see usage in ferdikoomen/openapi-typescript-codegen#494

I understand the need for this.

My suggestion would be to register middleware on build to overwrite the needed types and have the same middleware registered on runtime to do the conversions.

interface Property {
  type: string;
  format?: string;
  // other common properties to do conversions
  // can expand later
}

interface Middleware<UncovertedType, ConvertedType> {
  /**
    * path(s) to custom class/type to import for generated code types to work;
    * Could be left empty in the case of Date
    */
   import?: string|string[]|((property: Property) => (string|string[]));
   /**
    * This is the string representation of what will be set for the type in the generated files
    * Would be 'Date' for the case of Date
    */
   convertedType: string;
   /**
    * This will be used on build and runtime
    * Checks if the property should be converted
    * checkProperty: (prop) => prop.type === 'string' && prop.format === 'date-time',
    */
  checkProperty: (property: Property) => boolean
  /**
    * This will be used on runtime clients to convert data at the service layer.
    * property is the OpenAPI definition ;
    * 
    * Convert data would would only be called if checkProperty is true.
    * convertData: (data, _prop) => new Date(data),
    */
  convertData: (data: UncovertedType, property: Property) => ConvertedType;
}

const DateMiddleware: Middleware<string, Date> = {
  convertedType: 'Date',
  checkProperty: (prop) => prop.type === 'string' && prop.format === 'date-time',
  convertData: (data, _prop) => new Date(data),
}

Something like this approach would allow library consumers to extend the type-overriding system and offer a standard middleware API.

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024 3

Just a quick update, we will be addressing this after client packages are done #308

from openapi-ts.

seriouslag avatar seriouslag commented on May 24, 2024 2

Hey @seriouslag, this will never be a part of the client package. Client deals with sending requests, nothing more. However, we will support middleware, which would unlock this feature

Why add a property that breaks the contract of what is generated?

from openapi-ts.

Nick-Lucas avatar Nick-Lucas commented on May 24, 2024 2

Interesting, I was assuming the most performant way at runtime would be to generate a bunch of code like

class SomeService {
  async someMethod() {
    body = await fetch()
    
    # generate safe coersion for each date property
    if (body?.path?.to?.date) {
      body.path.to.date = new Date(body.path.to.date)
    }

    # also generate loops over any inner arrays etc

    return result
  }
}

Reading the schemas at runtime I guess wouldn't be crazy expensive but hard to beat this when you're doing codegen and know all the data structures up-front right? Or would it negatively affect bundle size for frontend clients?

from openapi-ts.

AnderssonPeter avatar AnderssonPeter commented on May 24, 2024 1

@AnderssonPeter can you explain what you mean by adding a new flag?

The useDateType flag, but i guess adding new types might not be that common, so it's maybe a none issue.

But the current implementation of useDateType does more harm than good, as the typescript definition says it will be a Date but it's still a string...

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024 1

@seriouslag yeah, something like that if we pass the original definition/schema to the runtime. Otherwise you have to loop through fields and check if string is convertible to date, which might not be a bulletproof approach

from openapi-ts.

seriouslag avatar seriouslag commented on May 24, 2024 1

@mrlubos well, just to be clear, i have no clue :D

Current state of affairs, when I add the --useDataType true the types get correctly generated, only the response is still a string. So I would like to return a Date object instead of a string.

Then I tried to create a interceptor like so

// DOES NOT WORK

OpenAPI.interceptors.response.use(async (response) => {

  console.log(response);

  return response; // <-- must return response

});



// client that is not authenticated

export const OpenClient = new Client({ ...OpenAPI, TOKEN: undefined });



// WORKS :D

OpenClient.request.config.interceptors.response.use(async (response) => {

  console.log(response);

  return response;

});

Issue here is, that its just as you say, you only get the response, not the correlating schema definition. So trying to hook up @seriouslag's suggestion has not worked yet.

Do you have an idea on how to get the correct schema in the response interceptor? Or do you know how I can transform the dates properly?

Thanks!!

Hey there, my suggestion was for inspiration to bring change to this library as the problem you are facing is the same others are facing. The current implementation of --useDateType causes confusion and generates broken code by default.

from openapi-ts.

jordanshatford avatar jordanshatford commented on May 24, 2024 1

NOTE, this config option will be changed:

export default {
  input: 'path/to/openapi.json',
  output: 'src/client',
  type: {
    dates: true,
  },
}

ref: https://heyapi.vercel.app/openapi-ts/migrating.html#removed-usedatetype

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024 1

@Nick-Lucas that's the trade-off to consider. Ideally we could measure both approaches and see which works better for us

from openapi-ts.

caesay avatar caesay commented on May 24, 2024 1

We are looking for type safety by using a generator, which means the "conversion from string to Date" needs to be done per property at code-gen time in my opinion. The idea of using an interceptor and changing anything that looks like a date, into a Date, is just really really bad. The generator knows that the source type is a string but the intended type is a Date so why do we need to guess at runtime?

Imagine the following config:

export default defineConfig({
  types: {
    "date-time": {
      typeName: "Date",
      typeConverter: "new Date($1)"
    } 
  }

This kind of model would allow far more flexibility and safety than a middleware. It unlocks new ways of date parsing:

export default defineConfig({
  types: {
    "date-time": {
      typeName: "moment",
      typeConverter: "moment($1)"
    } 
  }

Or even the ability to override arbitrary types to create a custom parser for a specific object type. Beyond the syntax above the only other thing I can see that's missing is the ability to specify where those types and/or helper functions come from so they can be imported.

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

Hey @seriouslag, this will never be a part of the client package. Client deals with sending requests, nothing more. However, we will support middleware, which would unlock this feature

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

Here's context for that feature ferdikoomen/openapi-typescript-codegen#494

from openapi-ts.

AnderssonPeter avatar AnderssonPeter commented on May 24, 2024

Can't the middleware also handle the type change?

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

@AnderssonPeter no. Types are generated during the build, the actual conversion needs to happen during runtime

from openapi-ts.

AnderssonPeter avatar AnderssonPeter commented on May 24, 2024

@AnderssonPeter no. Types are generated during the build, the actual conversion needs to happen during runtime

So for each new type I would need to modify the generator and add a new flag?

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

@AnderssonPeter can you explain what you mean by adding a new flag?

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

@AnderssonPeter that's why it's disabled by default. Please see usage in ferdikoomen/openapi-typescript-codegen#494

from openapi-ts.

reneweteling avatar reneweteling commented on May 24, 2024

Hey guys, so if i understand this correctly by adding the intercepted middleware, the output will be changed, only the type def will be the same, so the output would be

createdAt: new Date('2024-01-01'):string this isn't the way to go isn't it?

isn't it possible to register a interceptor or middleware in the config when you create the client from the OAS spec?

Also what is the propper way of installing this middleware?

I've tried this but that's obviously not working

OpenAPI.interceptors.response.use(async (response) => {
  await DateMiddleware(response); // async
  return response; // <-- must return response
});

Sorry, I'm a Ruby, Elxir dev, don't touch TS that much.

from openapi-ts.

reneweteling avatar reneweteling commented on May 24, 2024

Ok, so now im getting the correct date types in my models, only the output is still a string, guessing that's why we need the interceptor, only how do you hookt it all together?

openapi-ts -i ../server/swagger/swagger.yaml -o ./src/api/client --client fetch --exportSchemas true --name Client --useDateType true

This works :D

export type Token = {
  /**
   * Bearer token
   */
  accessToken: string;
  /**
   * Refresh token
   */
  refreshToken: string;
  /**
   * Token expiration time in seconds
   */
  expiresIn: number;
  /**
   * Creation time
   */
  createdAt: Date;
  /**
   * Token type
   */
  tokenType: string;
};

output of createdAt is still a string through.

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

That's right @reneweteling. What does your DateMiddleware look like? How do you know which endpoint + fields to transform?

from openapi-ts.

reneweteling avatar reneweteling commented on May 24, 2024

@mrlubos well, just to be clear, i have no clue :D

Current state of affairs, when I add the --useDataType true the types get correctly generated, only the response is still a string. So I would like to return a Date object instead of a string.

Then I tried to create a interceptor like so

// DOES NOT WORK
OpenAPI.interceptors.response.use(async (response) => {
  console.log(response);
  return response; // <-- must return response
});

// client that is not authenticated
export const OpenClient = new Client({ ...OpenAPI, TOKEN: undefined });

// WORKS :D
OpenClient.request.config.interceptors.response.use(async (response) => {
  console.log(response);
  return response;
});

Issue here is, that its just as you say, you only get the response, not the correlating schema definition. So trying to hook up @seriouslag's suggestion has not worked yet.

Do you have an idea on how to get the correct schema in the response interceptor? Or do you know how I can transform the dates properly?

Thanks!!

from openapi-ts.

reneweteling avatar reneweteling commented on May 24, 2024

@seriouslag yep, getting there :D thanks though, and sorry for the confusion

from openapi-ts.

Nick-Lucas avatar Nick-Lucas commented on May 24, 2024

Why can't transformation code be generated to "new Date()" at each site?

That would be far more valuable than changing the types and still having no solution to understand what values should become dates at runtime. It would be a killer feature for a package like this

from openapi-ts.

jordanshatford avatar jordanshatford commented on May 24, 2024

@Nick-Lucas that may be possible. I would need to look more into it, one issues is that I dont think each endpoint has that information right now. This would be a feature to add in

from openapi-ts.

Nick-Lucas avatar Nick-Lucas commented on May 24, 2024

Was just having a scan and it looks like the generator should have access to what's needed as the Model type is quite rich:

export interface Model extends Schema {
/**
* **Experimental.** Contains list of original refs so they can be used
* to access the schema from anywhere instead of relying on string name.
* This allows us to do things like detect type of ref.
*/
$refs: string[];
base: string;
deprecated?: boolean;
description: string | null;
enum: Enum[];
enums: Model[];
export:
| 'all-of'
| 'any-of'
| 'array'
| 'const'
| 'dictionary'
| 'enum'
| 'generic'
| 'interface'
| 'one-of'
| 'reference';
imports: string[];
link: Model | null;
name: string;
properties: Model[];
template: string | null;
type: string;
}

So hopefully "just" a case of iterating over the properties of the response during codegen and generating a transform for any date/date-time typed schemas in the response model

I don't have a ton of time this week but might at some stage be able to help contribute this if needed

from openapi-ts.

jordanshatford avatar jordanshatford commented on May 24, 2024

@mrlubos well, just to be clear, i have no clue :D

Current state of affairs, when I add the --useDataType true the types get correctly generated, only the response is still a string. So I would like to return a Date object instead of a string.

Then I tried to create a interceptor like so

// DOES NOT WORK

OpenAPI.interceptors.response.use(async (response) => {

  console.log(response);

  return response; // <-- must return response

});



// client that is not authenticated

export const OpenClient = new Client({ ...OpenAPI, TOKEN: undefined });



// WORKS :D

OpenClient.request.config.interceptors.response.use(async (response) => {

  console.log(response);

  return response;

});

Issue here is, that its just as you say, you only get the response, not the correlating schema definition. So trying to hook up @seriouslag's suggestion has not worked yet.

Do you have an idea on how to get the correct schema in the response interceptor? Or do you know how I can transform the dates properly?

Thanks!!

This issue with interceptors has been fixed. You can now pass an OpenAPI object with interceptors to the client and they will be used.

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

@Nick-Lucas I think what you're saying sounds about right and how we're thinking about approaching it. One distinction, Model is not available after generating the client. Instead, we could get this information from schemas.gen.ts, that's how we might solve this

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

@caesay see my note #181 (comment). Depending on the spec and payload size, this could really bloat your bundle

from openapi-ts.

caesay avatar caesay commented on May 24, 2024

@caesay see my note #181 (comment). Depending on the spec and payload size, this could really bloat your bundle

My suggestion won't hardly change the bundle size at all. If I understand your comment, you're suggesting the api schema is passed to interceptors (which I agree, would bloat your bundle). I'm not suggesting that. I'm suggesting that at code gen time we allow types to be converted using custom methods using the syntax I provided above. My suggestion does not require interceptors at all since the conversion is written into the generated api services.

from openapi-ts.

mrlubos avatar mrlubos commented on May 24, 2024

I am not sure I am following @caesay. Can you provide a sample service output with your approach? Don't worry about accuracy, just looking for the idea. How would it then know which field to transform?

from openapi-ts.

Nick-Lucas avatar Nick-Lucas commented on May 24, 2024

Interesting, I was assuming the most performant way at runtime would be to generate a bunch of code like

class SomeService {
  async someMethod() {
    body = await fetch()
    
    # generate safe coersion for each date property
    if (body?.path?.to?.date) {
      body.path.to.date = new Date(body.path.to.date)
    }

    # also generate loops over any inner arrays etc

    return result
  }
}

Reading the schemas at runtime I guess wouldn't be crazy expensive but hard to beat this when you're doing codegen and know all the data structures up-front right? Or would it negatively affect bundle size for frontend clients?

@mrlubos I think he's really referring to this but with the ability to override the templating and support moment, luxon, etc.

I doubt support for 3rd party date libs out the box needs to be a top priority, but bundle bloat shouldn't be an issue at all since this is opt in and the code to change these types is going to exist anyway if it's needed by a frontend, so that's a call that users can make. For BFFs it's also a non-concern

from openapi-ts.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.