Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"name" property is a reserved keyword in Typescript generator #1053

Closed
leigh-johnson opened this issue Dec 11, 2022 · 12 comments
Closed

"name" property is a reserved keyword in Typescript generator #1053

leigh-johnson opened this issue Dec 11, 2022 · 12 comments
Labels
bug Something isn't working TS generator Anything related to the TypeScript generator

Comments

@leigh-johnson
Copy link
Collaborator

leigh-johnson commented Dec 11, 2022

Describe the bug

I noticed one of my Typescript models is being generated with a reserved_name property instead of just name - even though name isn't a reserved keyword in Typescript.

How to Reproduce

version: 1.0.0-next.36

  1. Given the following schema:
  schemas:
    Camera:
      name: Camera
      type: object
      additionalProperties: false
      properties:
        index:
          type: integer
        name:
          type: string
        label:
          type: string
      required:
        - index
        - name
        - label
  1. Generate a Typescript model. Notice: reserved_name is rendered as the property name.
export interface Camera {
    index: number;
    reserved_name: string;
    label: string;
}

Here's my generator config, just in case that's relevant:

const generator = new TypeScriptFileGenerator({
  renderTypes: true,
  modelType: "interface",
  constraints: {
    propertyKey: typeScriptDefaultPropertyKeyConstraints({
      NAMING_FORMATTER: (name) => {
        return name;
      }
    })
  }
});
  1. Serializing Typescript model as JSON -> deserializing in Rust fails, since the Rust model expects this field to be called name
// Camera represents a Camera model.
#[derive(Clone, Debug, Deserialize, Eq, Hash, Ord, PartialEq, PartialOrd, Serialize)]
pub struct Camera {
    #[serde(rename="index")]
    pub index: i32,
    #[serde(rename="name")]
    pub name: String,
    #[serde(rename="label")]
    pub label: String,
}

impl Camera {
    pub fn new(index: i32, name: String, label: String) -> Camera {
        Camera {
            index,
            name,
            label,
        }
    }
}

Expected behavior

The following Typescript interface should be generated, given the schema above:

export interface Camera {
    index: number;
    name: string;
    label: string;
}
@leigh-johnson leigh-johnson added bug Something isn't working area/typescript Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. labels Dec 11, 2022
@leigh-johnson leigh-johnson added this to the Version 1.0.0 milestone Dec 11, 2022
@leigh-johnson
Copy link
Collaborator Author

name isn't in the reserved keyword list here, so I'm not sure where this is happening: 🤔
https://github.com/asyncapi/modelina/blob/master/src/generators/typescript/Constants.ts

@jonaslagoni
Copy link
Member

Always make sure you use one of the serialization presets otherwise there is no guarantee that the serialized models matches the expected JSON 🙂 The helper functions that are then included in your models always ensure that the right property names are used regardless of whether it's changed because of the language. The same goes for additional properties (from JSON Schema) as it needs to be unwrapped within the enclosing object.

The reserved keyword comes from JavaScript which is also ensured in TypeScript as often times the TypeScript models are transpiled to JS afterwards🙂

@leigh-johnson
Copy link
Collaborator Author

leigh-johnson commented Dec 11, 2022

Always make sure you use one of the serialization presets otherwise there is no guarantee that the serialized models matches the expected JSON slightly_smiling_face The helper functions that are then included in your models always ensure that the right property names are used regardless of whether it's changed because of the language. The same goes for additional properties (from JSON Schema) as it needs to be unwrapped within the enclosing object.

The reserved keyword comes from JavaScript which is also ensured in TypeScript as often times the TypeScript models are transpiled to JS afterwardsslightly_smiling_face

I'm using Nats.JSONCodec() and I'm not clear on how to use the serialization presets with NATS - when I deserialize the Typescript marshalled presets in another language (Rust), there are mismatched bytes. I think this is because the Typescript marshaled data is produced with : (extra space) as the separator - this matters when deserializing a stream of bytes. I'll file a separate issue when I have more time to write a test case.

Is name a reserved keyword in every circumstance in Javascript?

The reason why this jumped out as a bug to me is that I'm porting some OpenAPI schemas to AsyncAPI, and name isn't among the reserved keywords in the AbstractTypeScriptClientCodegen
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractTypeScriptClientCodegen.java#L290

Here's an example of an interface for a model that exports a name field, based on my OpenAPI Schemas:
https://github.com/bitsy-ai/printnanny-webapp/blob/main/clients/typescript/api.ts#L1413

Are you saying there's no way to use a schema with a property/field called name without having the property managled in an AsyncAPI schema?

@jonaslagoni
Copy link
Member

jonaslagoni commented Dec 12, 2022

using Nats.JSONCodec() and I'm not clear on how to use the serialization presets with NATS

This is how I use the models:

    const model = ...
    const jsonmodel : any = model.marshal();
    const dataToSend = codec.encode(jsonmodel);
    js.publish("mychannel", dataToSend);`;

This is from the TS NATS template: https://github.com/asyncapi/ts-nats-template/blob/81cce86ea4a07f2b7054d09956d86932a22548b5/components/channel/jetstreamPublish.js#L19

when I deserialize the Typescript marshalled presets in another language (Rust), there are mismatched bytes. I think this is because the Typescript marshaled data is produced with : (extra space) as the separator - this matters when deserializing a stream of bytes. I'll file a separate issue when I have more time to write a test case.

Yea, that should preferably not happen of course 😄

Is name a reserved keyword in every circumstance in Javascript?

It was taken from here: https://www.w3schools.com/js/js_reserved.asp

I can't off the top of my head remember which exact situation it cant be used.

Are you saying there's no way to use a schema with a property/field called name without having the property managled in an AsyncAPI schema?

Not if you are transpiling the models to js, no.

But it's entirely possible the JS keywords should only be seen as reserved given some new generator option, something like includeJavaScriptReservedKeywords, or what ever 😆 What do you think?

@leigh-johnson
Copy link
Collaborator Author

using Nats.JSONCodec() and I'm not clear on how to use the serialization presets with NATS

This is how I use the models:

    const model = ...
    const jsonmodel : any = model.marshal();
    const dataToSend = codec.encode(jsonmodel);
    js.publish("mychannel", dataToSend);`;

This is from the TS NATS template: https://github.com/asyncapi/ts-nats-template/blob/81cce86ea4a07f2b7054d09956d86932a22548b5/components/channel/jetstreamPublish.js#L19

when I deserialize the Typescript marshalled presets in another language (Rust), there are mismatched bytes. I think this is because the Typescript marshaled data is produced with : (extra space) as the separator - this matters when deserializing a stream of bytes. I'll file a separate issue when I have more time to write a test case.

Yea, that should preferably not happen of course smile

Is name a reserved keyword in every circumstance in Javascript?

It was taken from here: https://www.w3schools.com/js/js_reserved.asp

I can't off the top of my head remember which exact situation it cant be used.

Are you saying there's no way to use a schema with a property/field called name without having the property managled in an AsyncAPI schema?

Not if you are transpiling the models to js, no.

But it's entirely possible the JS keywords should only be seen as reserved given some new generator option, something like includeJavaScriptReservedKeywords, or what ever laughing What do you think?

Thanks for lending your brainpower on this one! 🙇‍♀️

I think a boolean option like includeJavaScriptReservedKeywords works for me.

Zooming out a bit, what do you think about reserved keywords being a preset? I know your design goals for Modelina are oriented toward modularity + customization. Coming from the OpenAPI universe, a few generators (like Dart IIRC) allow you to pass a custom reserved word list. That way you can remove certain terms or even add your own reserved terms.

@jonaslagoni
Copy link
Member

jonaslagoni commented Dec 16, 2022

Zooming out a bit, what do you think about reserved keywords being a preset? I know your design goals for Modelina are oriented toward modularity + customization. Coming from the OpenAPI universe, a few generators (like Dart IIRC) allow you to pass a custom reserved word list. That way you can remove certain terms or even add your own reserved terms.

I definitely think that would overload the presets, as they are not meant to be constraining the models even further, it should have already happened at the constraint phase, if not, then it's because we are missing some things in it's implementation. It also keeps it super clean as presets does one thing only, changing the generated model code 🙂

However, the thing about a custom reserved word list could definitely be an option to add! Feel free to raise this in a separate feature request! 🙇

@jonaslagoni jonaslagoni added TS generator Anything related to the TypeScript generator and removed area/typescript Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. labels Jan 20, 2023
@jonaslagoni
Copy link
Member

Removing this from the v1 milestone as this is a simple feature change and potential breaking change for changing default behavior.

@jonaslagoni jonaslagoni removed this from the Version 1.0.0 milestone Jan 20, 2023
@200Puls
Copy link
Contributor

200Puls commented Mar 16, 2023

We just stumbled across this with the property name "status" which was turned into "reservedStatus".

We have customized the List now by providing the propertykey constariner, with a custom set of keywords.

We stuck to the typescript keywords + this list for properties which seems to be quite a bit short than the standard list: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Lexical_grammar#reserved_words

I think the current list might be a bit to long.

  propertyKey: typeScriptDefaultPropertyKeyConstraints( {
        NO_RESERVED_KEYWORDS: (name) => {
            // custom keywords here 
  }

@jonaslagoni
Copy link
Member

Any suggestion for specific improvements we can take @200Puls ?

@200Puls
Copy link
Contributor

200Puls commented Mar 17, 2023

Hi @jonaslagoni,

I did some more research and my suggestion would be to use the official reserved word list of the Ecmascript specification for the javascript generator: https://tc39.es/ecma262/#sec-keywords-and-reserved-words

  • break
  • case
  • catch
  • class
  • const
  • continue
  • debugger
  • default
  • delete
  • do
  • else
  • enum
  • export
  • extends
  • false
  • finally
  • for
  • function
  • if
  • import
  • in
  • instanceof
  • new
  • null
  • return
  • super
  • switch
  • this
  • throw
  • true
  • try
  • typeof
  • var
  • void
  • while
  • with
  • yield
  • await

and the future reserved words:

  • enum
  • implements
  • interface
  • package
  • private
  • protected
  • public
  • eval
  • arguments

@binaryunary
Copy link

Just for info, we have also experienced this for properties named history, image, images, password.

@jonaslagoni
Copy link
Member

This is getting fixed by #1727

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working TS generator Anything related to the TypeScript generator
Projects
None yet
Development

No branches or pull requests

4 participants