xddq / schema2typebox

Creating TypeBox code from JSON schemas
MIT License
54 stars 12 forks source link

Quote object literal property names when required #36

Closed colonel-sanders closed 4 months ago

colonel-sanders commented 5 months ago

Quote object literal property names when required

In a JS/TS object literal, there are some restrictions on the definition of a property name. For example, this is not a valid object literal:

const cat = {
   9_lives: true
};

because a property name literal is not allowed to begin with a digit. To work around this, we can use a string literal for the property name:

const cat = {
  "9_lives": true
};

If a schema happens to define an object property that requires wrapping quotes, the generated Typescript code isn't currently syntactically correct.

Discussion link: https://github.com/xddq/schema2typebox/discussions/35

colonel-sanders commented 5 months ago

Forgot to mention: I did consider one alternative implementation here. Another valid approach to this problem would be to always use string literals for property names and then let the prettier run (as controlled by the quote-props setting) adjust it to match the user's preference.

xddq commented 4 months ago

Hey @colonel-sanders !

I see, thanks for the PR. The only problem I currently have is that I don't understand the regex at first sight.

I would tend towards the second approach, regex matching sometimes leaves some unwanted/unexpected cases open. However, I am wondering why it is not already applied yet, as "as_needed" seems to be the default setting for prettier. And for the case you mentioned it seems to be "needed". We already do "post-processing" with the imports plugin for prettier.

Will take a closer look probably this week.

colonel-sanders commented 4 months ago

However, I am wondering why it is not already applied yet, as "as_needed" seems to be the default setting for prettier. And for the case you mentioned it seems to be "needed". We already do "post-processing" with the imports plugin for prettier.

Thanks for taking a look, @xddq! I'd definitely be on-board for using string constants across the board; the default prettier settings will then rewrite that to the style most developers would expect.

Reading my earlier text, I think I wasn't abundantly clear about the failure mode here. To clarify: if I attempt to process this schema with schema2typebox@1.7.0:

{
  "$id": "https://example.com/interesting-prop-naming.json",
  "title": "InterestingPropNaming",
  "type": "object",
  "properties": {
    "@type": { "type": "string" }
  }
}

the prettier run actually errors out, because the input Typescript isn't valid:

SyntaxError: Property assignment expected. (6:51)
  4 |
  5 | export type InterestingPropNaming = Static<typeof InterestingPropNaming>
  6 | export const InterestingPropNaming = Type.Object({@type: Type.Optional(Type.String())}, {"$id":"https://example.com/interesting-prop-naming.json"})
    |                                                   ^
    at v (/.../node_modules/schema2typebox/node_modules/prettier/parser-typescript.js:1:14679)
xddq commented 4 months ago

@colonel-sanders did small changes to your code, but basically just used your approach and updated the function name. Will probably merge and release 1.7.1 in this or the next week. If you have time feel free to check the changes and approve/disapprove or comment.

I did drop your better handling when json schemas have escaped strings in favor of an easier to understand function. If this will be needed we can add it again, but the json schemas just seem weird like that.

Thanks again for the PR.

colonel-sanders commented 4 months ago

If you have time feel free to check the changes and approve/disapprove or comment.

Looks great. Thanks!